Re: master f0b0105: Hoist some byte-code checking out of eval

Re: master f0b0105: Hoist some byte-code checking out of eval

[Stefan Monnier]

>     Hoist some byte-code checking out of eval
[...]
>     * src/bytecode.c (Fbyte_code): Do sanity check and conditional
>     translation to unibyte here instead of each time the function is
>     executed.

Hi Paul.  I don't understand: AFAIK this function is only used in the
extremely rare case that Elisp does

    (byte-code "blabla")

So I think some of the checks you removed from `exec_byte_code` will
simply not be performed any more (at least not on
`byte-code-function-p` objects).


        Stefan

Re: master f0b0105: Hoist some byte-code checking out of eval

[Paul Eggert]

On 5/20/20 5:03 AM, Stefan Monnier wrote:
> AFAIK this function is only used in the
> extremely rare case that Elisp does
> 
>      (byte-code "blabla")
> 
> So I think some of the checks you removed from `exec_byte_code` will
> simply not be performed any more (at least not on
> `byte-code-function-p` objects).

Thanks for reviewing the patch.

What checks do you have in mind? Formerly, exec_byte_code did this:

   CHECK_STRING (bytestr); 

   CHECK_VECTOR (vector); 

   CHECK_FIXNAT (maxdepth);
   if (STRING_MULTIBYTE (bytestr)) 

     bytestr = Fstring_as_unibyte (bytestr);

Now, byte-code does this:

   if (! (STRINGP (bytestr) && VECTORP (vector) && FIXNATP (maxdepth))) 

     error ("Invalid byte-code"); 
 

   if (STRING_MULTIBYTE (bytestr)) 

     bytestr = Fstring_as_unibyte (bytestr);

and to my eye this does everything that exec_byte_code used to do.

Re: master f0b0105: Hoist some byte-code checking out of eval

[Stefan Monnier]

>> AFAIK this function is only used in the
>> extremely rare case that Elisp does
>>      (byte-code "blabla")
>> So I think some of the checks you removed from `exec_byte_code` will
>> simply not be performed any more (at least not on
>> `byte-code-function-p` objects).
>
> Thanks for reviewing the patch.
>
> What checks do you have in mind? Formerly, exec_byte_code did this:
>
>   CHECK_STRING (bytestr); 
>   CHECK_VECTOR (vector); 
>   CHECK_FIXNAT (maxdepth);
>   if (STRING_MULTIBYTE (bytestr)) 
>     bytestr = Fstring_as_unibyte (bytestr);
>
> Now, byte-code does this:
>
>   if (! (STRINGP (bytestr) && VECTORP (vector) && FIXNATP (maxdepth))) 
>     error ("Invalid byte-code");  
>
>   if (STRING_MULTIBYTE (bytestr)) 
>     bytestr = Fstring_as_unibyte (bytestr);
>
> and to my eye this does everything that exec_byte_code used to do.

My point is that 99% of the calls to `exec_byte_code` don't go through
`Fbyte_code`.


        Stefan

Re: master f0b0105: Hoist some byte-code checking out of eval

[Paul Eggert]

On 5/20/20 4:21 PM, Stefan Monnier wrote:
> My point is that 99% of the calls to `exec_byte_code` don't go through
> `Fbyte_code`.

Those other calls are also supposed to arrange for the relevant checks 
to be done, typically when the objects given to exec_byte_code were 
created. It saves some time to do the checks once on object creation 
rather than every time the byte code is called.

At least, that was the idea. Did I miss a path of execution that skips 
the checks?

Re: master f0b0105: Hoist some byte-code checking out of eval

[Stefan Monnier]

>> My point is that 99% of the calls to `exec_byte_code` don't go through
>> `Fbyte_code`.
> Those other calls are also supposed to arrange for the relevant checks to be
> done, typically when the objects given to exec_byte_code were created. It
> saves some time to do the checks once on object creation rather than every
> time the byte code is called.

I understand, but looking at the patch and the comment message, I see
that you moved the checks from exec_byte_code to Fbyte_code and moved
duplicated *some* of the checks in make_byte_code but not all of them.

I probably missed something,


        Stefan

Re: master f0b0105: Hoist some byte-code checking out of eval

[Paul Eggert]

On 5/20/20 7:30 PM, Stefan Monnier wrote:

> I understand, but looking at the patch and the comment message, I see
> that you moved the checks from exec_byte_code to Fbyte_code and moved
> duplicated *some* of the checks in make_byte_code but not all of them.

Oh, now that you mention it I do see something missing: Fbyte_code 
converts bytestr to unibyte whereas Fmake_byte_code merely checks that 
bytestr is unibyte, and I suppose it should be consistent with 
Fbyte_code here. I'll look into fixing that. This should matter only 
when dealing with Emacs 20.2-and-older bytecode strings (do we still 
need to worry about that?).

You wrote "*some* .. but not all", which vaguely implies that I was 
missing more than one check. Am I missing something else here?

Re: master f0b0105: Hoist some byte-code checking out of eval

[Stefan Monnier]

> Oh, now that you mention it I do see something missing: Fbyte_code converts
> bytestr to unibyte whereas Fmake_byte_code merely checks that bytestr is
> unibyte, and I suppose it should be consistent with Fbyte_code here. I'll
> look into fixing that. This should matter only when dealing with Emacs
> 20.2-and-older bytecode strings (do we still need to worry about that?).
>
> You wrote "*some* .. but not all", which vaguely implies that I was missing
> more than one check. Am I missing something else here?

I just saw that the checks were not textually the same (I expected
basically "exactly" the same code to be duplicated to `Fbyte_code` and
`make-byte-code`; not sure if `Ffetch_bytecode` also needs some of that
treatment).


        Stefan