GNU bug report logs -
#41242
Port feature/native-comp to Windows
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41242 in the body.
You can then email your comments to 41242 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 19:28:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Nicolas Bértolo <nicolasbertolo <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Wed, 13 May 2020 19:28:01 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
The attached patches contain changes to make the feature/native-comp branch work
on Windows.
There are a few remaining issues:
* The loading process is very slow. This is especially annoying when coupled
with autoloading. For example, autoloading Helm may stall Emacs for 5 seconds
in my machine.
I have thought a possible solution to this problem: load the byte-compiled
file and put the native-compiled version in a list. Then load that list one by
one on an idle-timer, that way the UI freezes should be minimized. This could
reuse the "late loading" machinery implemented for deferred compilation.
* `package-delete` fails because it tries to delete the .eln file via
`delete-file`. This is impossible in Windows because it's generally impossible
to delete files that have an open HANDLE in the system.
Three solutions have crossed my mind:
- Edit `package-delete` to put the eln on a list of files that should be
deleted when Emacs is closed.
- Implement an unloading mechanism for native-compiled files.
- Copy eln files to temporary files and load those temporary files instead.
This means that deleting the original eln file is possible.
I'd prefer the second option, but I have a semi-working patch for the third
option.
* The `emacs_dir` environment variable needs to be set when loading the dump
file. It is necessary for `expand-file-name`, which calls emacs_root_dir(). I
haven't investigated why this is necessary yet. One workaround would be to use
GetModuleFileName() to get the path to emacs.exe when `emacs_dir` is not set.
[0001-HACK-Ensure-the-emacs_root_dir-function-does-not-cra.patch (application/octet-stream, attachment)]
[0002-Do-not-block-SIGIO-in-platforms-that-don-t-have-it.patch (application/octet-stream, attachment)]
[0003-Handle-setjmp-taking-two-arguments-in-Windows.patch (application/octet-stream, attachment)]
[0004-Handle-LISP_WORDS_ARE_POINTERS-and-CHECK_LISP_OBJECT.patch (application/octet-stream, attachment)]
[0005-Remove-a-layer-of-indirection-for-access-to-pure-sto.patch (application/octet-stream, attachment)]
[0006-Workaround-the-32768-chars-command-line-limit-in-Win.patch (application/octet-stream, attachment)]
[0007-Load-libgccjit-dynamically-in-Windows.patch (application/octet-stream, attachment)]
[0008-Windows-Use-NUMBER_OF_PROCESSORS-environment-variabl.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 19:37:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Wed, 13 May 2020 16:26:57 -0300
>
> * The loading process is very slow. This is especially annoying when coupled
> with autoloading. For example, autoloading Helm may stall Emacs for 5 seconds
> in my machine.
Did you manage to understand why? How many bytes of *.eln files does
Emacs load while autoloading Helm, for example?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 19:41:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Wed, 13 May 2020 16:26:57 -0300
>
> * `package-delete` fails because it tries to delete the .eln file via
> `delete-file`. This is impossible in Windows because it's generally impossible
> to delete files that have an open HANDLE in the system.
Is that the handle from LoadLibrary? If so, cannot we close the
handle once the .eln file is loaded?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 19:57:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> The attached patches contain changes to make the feature/native-comp branch work
> on Windows.
>
> There are a few remaining issues:
>
> * The loading process is very slow. This is especially annoying when coupled
> with autoloading. For example, autoloading Helm may stall Emacs for 5 seconds
> in my machine.
>
> I have thought a possible solution to this problem: load the byte-compiled
> file and put the native-compiled version in a list. Then load that list one by
> one on an idle-timer, that way the UI freezes should be minimized. This could
> reuse the "late loading" machinery implemented for deferred compilation.
That's an option, but before implementing any machinery base on a timers
we need to profile to understand where we are loosing the time.
The timer is a mitigation but will still freeze the editor so it's a
last resource.
What is the state of your paperwork? I guess still ongoing am I
correct?
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 20:03:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Did you manage to understand why? How many bytes of *.eln files does
> Emacs load while autoloading Helm, for example?
I don't know how to measure that, sorry. AFAIK Emacs just maps many subr to the
correct function pointers and the OS takes care of loading the appropriate code
on page faults.
My guess is that autoloading triggers a long series of eln loading operations,
that, as a group, are very expensive.
Do you know what profiler I could use to check what Emacs is doing?
> Is that the handle from LoadLibrary? If so, cannot we close the
> handle once the .eln file is loaded?
Exactly. No, calling FreeLibrary would unload the file from the address space.
El mié., 13 may. 2020 a las 16:40, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
>
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Wed, 13 May 2020 16:26:57 -0300
> >
> > * `package-delete` fails because it tries to delete the .eln file via
> > `delete-file`. This is impossible in Windows because it's generally impossible
> > to delete files that have an open HANDLE in the system.
>
> Is that the handle from LoadLibrary? If so, cannot we close the
> handle once the .eln file is loaded?
>
> Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 20:05:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> What is the state of your paperwork? I guess still ongoing am I
> correct?
I have already sent the signed form. I haven't got a response yet.
El mié., 13 may. 2020 a las 16:56, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> > The attached patches contain changes to make the feature/native-comp branch work
> > on Windows.
> >
> > There are a few remaining issues:
> >
> > * The loading process is very slow. This is especially annoying when coupled
> > with autoloading. For example, autoloading Helm may stall Emacs for 5 seconds
> > in my machine.
> >
> > I have thought a possible solution to this problem: load the byte-compiled
> > file and put the native-compiled version in a list. Then load that list one by
> > one on an idle-timer, that way the UI freezes should be minimized. This could
> > reuse the "late loading" machinery implemented for deferred compilation.
>
> That's an option, but before implementing any machinery base on a timers
> we need to profile to understand where we are loosing the time.
>
> The timer is a mitigation but will still freeze the editor so it's a
> last resource.
>
> What is the state of your paperwork? I guess still ongoing am I
> correct?
>
> Thanks
>
> Andrea
>
> --
> akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 20:09:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
>> Date: Wed, 13 May 2020 16:26:57 -0300
>>
>> * `package-delete` fails because it tries to delete the .eln file via
>> `delete-file`. This is impossible in Windows because it's generally impossible
>> to delete files that have an open HANDLE in the system.
>
> Is that the handle from LoadLibrary? If so, cannot we close the
> handle once the .eln file is loaded?
AFAIU we cannot close the handle till we have code around (compiled Lisp
function) coming from the eln.
Andrea
PS BTW I see now we never close the handle but this is a bug, I'll fix now.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 20:28:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Andrea Corallo <akrl <at> sdf.org> writes:
> PS BTW I see now we never close the handle but this is a bug, I'll fix now.
Ah no we do it already. Missed that sorry I'm fused.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 13 May 2020 22:26:04 GMT)
Full text and
rfc822 format available.
Message #29 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Did you manage to understand why? How many bytes of *.eln files does
>> Emacs load while autoloading Helm, for example?
>
> I don't know how to measure that, sorry. AFAIK Emacs just maps many subr to the
> correct function pointers and the OS takes care of loading the appropriate code
> on page faults.
Loading elns quite stress also the reader that is still used to
deserialize all objects except functions.
The fact that the load is that slower on Windows seems to indicate that
the equivalent dlopen dlsym are less performant than the other systems
we have tried (the reader should be quite the same).
That said there must be a way to profile on Windows so we get a picture
of what is going on.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 10:19:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> * `package-delete` fails because it tries to delete the .eln file via
> `delete-file`. This is impossible in Windows because it's generally impossible
> to delete files that have an open HANDLE in the system.
>
> Three solutions have crossed my mind:
>
> - Edit `package-delete` to put the eln on a list of files that should be
> deleted when Emacs is closed.
>
> - Implement an unloading mechanism for native-compiled files.
>
> - Copy eln files to temporary files and load those temporary files instead.
> This means that deleting the original eln file is possible.
>
> I'd prefer the second option, but I have a semi-working patch for the third
> option.
I don't think the second is an option. The unload machanism is already
in place but is GC driven, I don't see how else it could work.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 10:46:01 GMT)
Full text and
rfc822 format available.
Message #35 received at submit <at> debbugs.gnu.org (full text, mbox):
On May 14, 2020 1:18:26 PM GMT+03:00, Andrea Corallo <akrl <at> sdf.org> wrote:
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> > * `package-delete` fails because it tries to delete the .eln file
> via
> > `delete-file`. This is impossible in Windows because it's
> generally impossible
> > to delete files that have an open HANDLE in the system.
> >
> > Three solutions have crossed my mind:
> >
> > - Edit `package-delete` to put the eln on a list of files that
> should be
> > deleted when Emacs is closed.
> >
> > - Implement an unloading mechanism for native-compiled files.
> >
> > - Copy eln files to temporary files and load those temporary files
> instead.
> > This means that deleting the original eln file is possible.
> >
> > I'd prefer the second option, but I have a semi-working patch for
> the third
> > option.
>
> I don't think the second is an option. The unload machanism is
> already
> in place but is GC driven, I don't see how else it could work.
Windows doesn't let you delete a shared library that's loaded by a process, but it does let you rename it. So I think the solution would be to rename the .eln file to something like .eln.old, and then let the GC driven unload machinery to delete that old file.
Btw, what happens if more than one Emacs session have the same .eln file loaded, and one of them wants to recompile it?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 10:46:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 11:18:02 GMT)
Full text and
rfc822 format available.
Message #41 received at submit <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I don't think the second is an option. The unload machanism is
>> already
>> in place but is GC driven, I don't see how else it could work.
>
> Windows doesn't let you delete a shared library that's loaded by a
> process, but it does let you rename it. So I think the solution would
> be to rename the .eln file to something like .eln.old, and then let
> the GC driven unload machinery to delete that old file.
Do we have guarantees that each object is collected before Emacs is
eventually closed? I thought is not the case.
> Btw, what happens if more than one Emacs session have the same .eln file loaded, and one of them wants to recompile it?
Now to avoid this problem we always delete the file before recompiling.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 11:18:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 13:43:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Wed, 13 May 2020 17:01:59 -0300
> Cc: 41242 <at> debbugs.gnu.org
>
> > Did you manage to understand why? How many bytes of *.eln files does
> > Emacs load while autoloading Helm, for example?
>
> I don't know how to measure that, sorry. AFAIK Emacs just maps many subr to the
> correct function pointers and the OS takes care of loading the appropriate code
> on page faults.
>
> My guess is that autoloading triggers a long series of eln loading operations,
> that, as a group, are very expensive.
>
> Do you know what profiler I could use to check what Emacs is doing?
I'd begin with Emacs's built-in "M-x profiler-start". After invoking
that, start the Helm loading command, and when it ends, invoke
profiler-report. Post the resulting profile fully expanded, and maybe
that will give some clues about what to examine next.
It will also be useful to have a comparable measurement from
GNU/Linux, so that we could compare the profiles and the elapsed
times.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 14:33:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: bug-gnu-emacs <at> gnu.org,
> Nicolas Bértolo
> <nicolasbertolo <at> gmail.com>,
> 41242 <at> debbugs.gnu.org
> Date: Thu, 14 May 2020 11:17:11 +0000
>
> > Windows doesn't let you delete a shared library that's loaded by a
> > process, but it does let you rename it. So I think the solution would
> > be to rename the .eln file to something like .eln.old, and then let
> > the GC driven unload machinery to delete that old file.
>
> Do we have guarantees that each object is collected before Emacs is
> eventually closed? I thought is not the case.
I don't know enough about the "GC driven unload" you mentioned, but if
that is not bulletproof enough, we could add a kill-emacs hook to take
care of this. And if push comes to shove, we could use a Windows API
that causes a file to be deleted when the last handle on it is closed,
but that would add complexity, so I think we should try easier ways
first.
> > Btw, what happens if more than one Emacs session have the same .eln file loaded, and one of them wants to recompile it?
>
> Now to avoid this problem we always delete the file before recompiling.
But that's unportable, and won't work on Windows, for the same reasons
as the issue we are discussing here. Or am I missing something?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 15:04:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Andrea Corallo <akrl <at> sdf.org>
>> Cc: bug-gnu-emacs <at> gnu.org,
>> Nicolas Bértolo
>> <nicolasbertolo <at> gmail.com>,
>> 41242 <at> debbugs.gnu.org
>> Date: Thu, 14 May 2020 11:17:11 +0000
>>
>> > Windows doesn't let you delete a shared library that's loaded by a
>> > process, but it does let you rename it. So I think the solution would
>> > be to rename the .eln file to something like .eln.old, and then let
>> > the GC driven unload machinery to delete that old file.
>>
>> Do we have guarantees that each object is collected before Emacs is
>> eventually closed? I thought is not the case.
>
> I don't know enough about the "GC driven unload" you mentioned, but if
> that is not bulletproof enough, we could add a kill-emacs hook to take
> care of this. And if push comes to shove, we could use a Windows API
> that causes a file to be deleted when the last handle on it is closed,
> but that would add complexity, so I think we should try easier ways
> first.
Now simply when the compilation unit object is collected, the handle is
closed. The compilation unit is collected when is not reachable by any
of the functions defined into.
Loading a new compilation unit B that redefines the same functions
defined by A does not guarantees much, some of the old definitions of A
could be still in use somewhere, in that case A will not be collected.
So yeah I think we need a specific mechanism (kill-emacs hook as you
suggest) to do the cleanup when Emacs goes down.
>> > Btw, what happens if more than one Emacs session have the same .eln file loaded, and one of them wants to recompile it?
>>
>> Now to avoid this problem we always delete the file before recompiling.
>
> But that's unportable, and won't work on Windows, for the same reasons
> as the issue we are discussing here. Or am I missing something?
No you are right, I don't use Windows since forever, I discovered from
this thread is not portable. I guess we'll need to rename also here.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 16:25:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> And if push comes to shove, we could use a Windows API
> that causes a file to be deleted when the last handle on it is closed,
> but that would add complexity, so I think we should try easier ways
> first.
If you are talking about FILE_FLAG_DELETE_ON_CLOSE:
I have tried that in the patch I have for the third suggestion.
When Emacs closes normally the temporary files are not deleted.
I don't know why that is. It is as if the LoadLibrary function disabled
the delete on close flag.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 16:52:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Loading a new compilation unit B that redefines the same functions
> defined by A does not guarantees much, some of the old definitions of A
> could be still in use somewhere, in that case A will not be collected.
> So yeah I think we need a specific mechanism (kill-emacs hook as you
> suggest) to do the cleanup when Emacs goes down.
I was thinking about something like this:
1) Call `native-comp-unload`.
2) This should inspect the eln file and put all the subrs defined in it on a
list. This should also copy the original bytecode from the eln file and store it
somewhere.
3) Then `garbage-collect` is called. This should find all references to the
subrs in the list and swap them atomically for references to functions
from the bytecode.
4) After the previous step the GC should be able to collect the DLL handle
knowing that no references to it remain.
What do you think?
> No you are right, I don't use Windows since forever, I discovered from
> this thread is not portable. I guess we'll need to rename also here.
But someone needs to delete the old eln file. Let's say that we use
GetModuleFileNameA() to know if another Emacs instance has decided to rename the
eln file and then we delete it on close if its suffix is ".eln.old".
This algorithm has a big race condition though. If Emacs1 renames the file after
Emacs2 has checked that it has not been renamed, then the file won't be deleted.
If we put the "old eln" in the $TEMP folder this may not be a big issue though.
Nicolas.
El jue., 14 may. 2020 a las 12:03, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Andrea Corallo <akrl <at> sdf.org>
> >> Cc: bug-gnu-emacs <at> gnu.org,
> >> Nicolas Bértolo
> >> <nicolasbertolo <at> gmail.com>,
> >> 41242 <at> debbugs.gnu.org
> >> Date: Thu, 14 May 2020 11:17:11 +0000
> >>
> >> > Windows doesn't let you delete a shared library that's loaded by a
> >> > process, but it does let you rename it. So I think the solution would
> >> > be to rename the .eln file to something like .eln.old, and then let
> >> > the GC driven unload machinery to delete that old file.
> >>
> >> Do we have guarantees that each object is collected before Emacs is
> >> eventually closed? I thought is not the case.
> >
> > I don't know enough about the "GC driven unload" you mentioned, but if
> > that is not bulletproof enough, we could add a kill-emacs hook to take
> > care of this. And if push comes to shove, we could use a Windows API
> > that causes a file to be deleted when the last handle on it is closed,
> > but that would add complexity, so I think we should try easier ways
> > first.
>
> Now simply when the compilation unit object is collected, the handle is
> closed. The compilation unit is collected when is not reachable by any
> of the functions defined into.
>
> Loading a new compilation unit B that redefines the same functions
> defined by A does not guarantees much, some of the old definitions of A
> could be still in use somewhere, in that case A will not be collected.
>
> So yeah I think we need a specific mechanism (kill-emacs hook as you
> suggest) to do the cleanup when Emacs goes down.
>
> >> > Btw, what happens if more than one Emacs session have the same .eln file loaded, and one of them wants to recompile it?
> >>
> >> Now to avoid this problem we always delete the file before recompiling.
> >
> > But that's unportable, and won't work on Windows, for the same reasons
> > as the issue we are discussing here. Or am I missing something?
>
> No you are right, I don't use Windows since forever, I discovered from
> this thread is not portable. I guess we'll need to rename also here.
>
> Andrea
>
> --
> akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:15:02 GMT)
Full text and
rfc822 format available.
Message #62 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: nicolasbertolo <at> gmail.com, 41242 <at> debbugs.gnu.org
> Date: Thu, 14 May 2020 15:03:08 +0000
>
> Now simply when the compilation unit object is collected, the handle is
> closed. The compilation unit is collected when is not reachable by any
> of the functions defined into.
>
> Loading a new compilation unit B that redefines the same functions
> defined by A does not guarantees much, some of the old definitions of A
> could be still in use somewhere, in that case A will not be collected.
>
> So yeah I think we need a specific mechanism (kill-emacs hook as you
> suggest) to do the cleanup when Emacs goes down.
Some list of files to remove and a function to remove them at exit
should be sufficient.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:22:02 GMT)
Full text and
rfc822 format available.
Message #65 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 13:24:02 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > And if push comes to shove, we could use a Windows API
> > that causes a file to be deleted when the last handle on it is closed,
> > but that would add complexity, so I think we should try easier ways
> > first.
>
> If you are talking about FILE_FLAG_DELETE_ON_CLOSE:
> I have tried that in the patch I have for the third suggestion.
> When Emacs closes normally the temporary files are not deleted.
> I don't know why that is. It is as if the LoadLibrary function disabled
> the delete on close flag.
Like I said: let's try more traditional ways first ;-)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:30:02 GMT)
Full text and
rfc822 format available.
Message #68 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 13:50:48 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> 1) Call `native-comp-unload`.
>
> 2) This should inspect the eln file and put all the subrs defined in it on a
> list. This should also copy the original bytecode from the eln file and store it
> somewhere.
>
> 3) Then `garbage-collect` is called. This should find all references to the
> subrs in the list and swap them atomically for references to functions
> from the bytecode.
>
> 4) After the previous step the GC should be able to collect the DLL handle
> knowing that no references to it remain.
>
> What do you think?
Do we really need this complexity?
> > No you are right, I don't use Windows since forever, I discovered from
> > this thread is not portable. I guess we'll need to rename also here.
>
> But someone needs to delete the old eln file. Let's say that we use
> GetModuleFileNameA() to know if another Emacs instance has decided to rename the
> eln file and then we delete it on close if its suffix is ".eln.old".
>
> This algorithm has a big race condition though. If Emacs1 renames the file after
> Emacs2 has checked that it has not been renamed, then the file won't be deleted.
Why do you need to check if it's renamed? Just rename always.
> If we put the "old eln" in the $TEMP folder this may not be a big issue though.
It is not good to move to $TEMP because that one could be on a
different volume, and Windows won't let you do that with a DLL that is
loaded into a process.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:35:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Loading a new compilation unit B that redefines the same functions
>> defined by A does not guarantees much, some of the old definitions of A
>> could be still in use somewhere, in that case A will not be collected.
>
>> So yeah I think we need a specific mechanism (kill-emacs hook as you
>> suggest) to do the cleanup when Emacs goes down.
>
> I was thinking about something like this:
>
> 1) Call `native-comp-unload`.
>
> 2) This should inspect the eln file and put all the subrs defined in it on a
> list. This should also copy the original bytecode from the eln file and store it
> somewhere.
>
> 3) Then `garbage-collect` is called. This should find all references to the
> subrs in the list and swap them atomically for references to functions
> from the bytecode.
>
> 4) After the previous step the GC should be able to collect the DLL handle
> knowing that no references to it remain.
>
> What do you think?
It can't work for the following reasons:
1- eln files do not retain the orginal function bytecode.
2- in any point of the code you can get the symbol-function of a defined
function and leak it everywhere. We can't technically "swap"
functions definitions, the best we do is just redefine them that is
set a certain symbol-function (what late load does). The old
definitions can always still be used if the referennce is still
present somewhere. Even worst the function you want to remove could
be active in the stack!
One option would be to add a field into the compilation unit object like
'pending_for_removal', each time any of this is unloaded by GC the file
is removed if the field suggests that. At the very last when Emacs is
closing we go through all the compilation units and remove the files
that are still pending.
>> No you are right, I don't use Windows since forever, I discovered from
>> this thread is not portable. I guess we'll need to rename also here.
>
> But someone needs to delete the old eln file. Let's say that we use
> GetModuleFileNameA() to know if another Emacs instance has decided to rename the
> eln file and then we delete it on close if its suffix is ".eln.old".
>
> This algorithm has a big race condition though. If Emacs1 renames the file after
> Emacs2 has checked that it has not been renamed, then the file won't be deleted.
> If we put the "old eln" in the $TEMP folder this may not be a big issue though.
Yes renaming for me here was moving into a temporary folder.
This could be a very simple option also for the first problem if we
accept we can leave some file there from time to time.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:36:01 GMT)
Full text and
rfc822 format available.
Message #74 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Why do you need to check if it's renamed? Just rename always.
You need to see if the file has been renamed because someone needs
to delete the file
> It is not good to move to $TEMP because that one could be on a
> different volume, and Windows won't let you do that with a DLL that is
> loaded into a process.
Touché.
El jue., 14 may. 2020 a las 14:28, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
>
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Thu, 14 May 2020 13:50:48 -0300
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
> >
> > 1) Call `native-comp-unload`.
> >
> > 2) This should inspect the eln file and put all the subrs defined in it on a
> > list. This should also copy the original bytecode from the eln file and store it
> > somewhere.
> >
> > 3) Then `garbage-collect` is called. This should find all references to the
> > subrs in the list and swap them atomically for references to functions
> > from the bytecode.
> >
> > 4) After the previous step the GC should be able to collect the DLL handle
> > knowing that no references to it remain.
> >
> > What do you think?
>
> Do we really need this complexity?
>
> > > No you are right, I don't use Windows since forever, I discovered from
> > > this thread is not portable. I guess we'll need to rename also here.
> >
> > But someone needs to delete the old eln file. Let's say that we use
> > GetModuleFileNameA() to know if another Emacs instance has decided to rename the
> > eln file and then we delete it on close if its suffix is ".eln.old".
> >
> > This algorithm has a big race condition though. If Emacs1 renames the file after
> > Emacs2 has checked that it has not been renamed, then the file won't be deleted.
>
> Why do you need to check if it's renamed? Just rename always.
>
> > If we put the "old eln" in the $TEMP folder this may not be a big issue though.
>
> It is not good to move to $TEMP because that one could be on a
> different volume, and Windows won't let you do that with a DLL that is
> loaded into a process.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:52:01 GMT)
Full text and
rfc822 format available.
Message #77 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> 1- eln files do not retain the orginal function bytecode.
That should be easy to change.
> in any point of the code you can get the symbol-function of a defined
function and leak it everywhere.
This is why I said the GC should do it. It should be able to find all
references.
> We can't technically "swap" functions definitions, the best we do is just
> redefine them that is set a certain symbol-function (what late load does).
When the GC finds a Lisp_Object that points to a subr we want to unload,
it should replace it with one that points to its bytecode equivalent version.
> Even worst the function you want to remove could be active in the stack!
You are right. I hadn't considered this. It can still be done, but would need to
wait until the function finishes.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 17:58:02 GMT)
Full text and
rfc822 format available.
Message #80 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 14:35:36 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > Why do you need to check if it's renamed? Just rename always.
>
> You need to see if the file has been renamed because someone needs
> to delete the file
If you always rename, then you need to always delete, no?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:01:01 GMT)
Full text and
rfc822 format available.
Message #83 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> If you always rename, then you need to always delete, no?
The Emacs instance that renames the file may not be able to delete the file
because another Emacs instance may keep an open handle.
It is this last Emacs instance that should delete the file, not the
one that renamed it.
El jue., 14 may. 2020 a las 14:57, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
>
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Thu, 14 May 2020 14:35:36 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > > Why do you need to check if it's renamed? Just rename always.
> >
> > You need to see if the file has been renamed because someone needs
> > to delete the file
>
> If you always rename, then you need to always delete, no?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:14:02 GMT)
Full text and
rfc822 format available.
Message #86 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> 1- eln files do not retain the orginal function bytecode.
>
> That should be easy to change.
Sure but we don't want to do that because there's no reason to bloat the
eln.
>> in any point of the code you can get the symbol-function of a defined
> function and leak it everywhere.
>
> This is why I said the GC should do it. It should be able to find all
> references.
Yes but I can't *remove* objects that are referenced niether swap them.
See below.
>> We can't technically "swap" functions definitions, the best we do is just
>> redefine them that is set a certain symbol-function (what late load does).
>
> When the GC finds a Lisp_Object that points to a subr we want to unload,
> it should replace it with one that points to its bytecode equivalent version.
No it cannot, if an object has been tested to say satisfy a predicate
you cannot change it for another one. It would break tons of code and
make the system totally un-debuggable.
>> Even worst the function you want to remove could be active in the stack!
>
> You are right. I hadn't considered this. It can still be done, but would need to
> wait until the function finishes.
Yes, but when are all functions you want to get rid deactivated?
Here we are really complexifying a problem that is not. IMO renaming
and having a list to do the clean-up are sufficient tools to solve it.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:30:02 GMT)
Full text and
rfc822 format available.
Message #89 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> If you always rename, then you need to always delete, no?
>
> The Emacs instance that renames the file may not be able to delete the file
> because another Emacs instance may keep an open handle.
>
> It is this last Emacs instance that should delete the file, not the
> one that renamed it.
Mmmhh, probably also a lock file should be deposed by each Emacs using
the file? That way the last session closing could check and remove?
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:31:01 GMT)
Full text and
rfc822 format available.
Message #92 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 15:00:14 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > If you always rename, then you need to always delete, no?
>
> The Emacs instance that renames the file may not be able to delete the file
> because another Emacs instance may keep an open handle.
>
> It is this last Emacs instance that should delete the file, not the
> one that renamed it.
Then I guess on Windows we will have to live with the limitation that
a .eln file used by another session cannot be recompiled.
(I must confess that the idea to use libgccjit and shared libraries
for native compilation looks less and less attractive to me due to
these complications.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:36:02 GMT)
Full text and
rfc822 format available.
Message #95 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> (I must confess that the idea to use libgccjit and shared libraries
> for native compilation looks less and less attractive to me due to
> these complications.)
Come on, do we get demotivated that early!? :) :)
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:41:01 GMT)
Full text and
rfc822 format available.
Message #98 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Here we are really complexifying a problem that is not. IMO renaming
> and having a list to do the clean-up are sufficient tools to solve it.
You're right. I just think that renaming and a list do not guarantee that we
always delete the files when we should.
Consider this case:
* Emacs1 and Emacs2 are two Emacs instances that load the
same foo.eln file.
* Emacs1 decides to recompile foo.el. To do that it renames foo.eln to
foo.eln.old. It creates a new foo.eln and tries to delete foo.eln.old. It
fails because Emacs2 has an open handle to it.
* When Emacs2 closes it realizes that foo.eln has been renamed to foo.eln.old
and deletes it.
That is the good case.
If we are unlucky this is what may happen:
* Emacs2 begins to close. It checks that foo.eln has not been renamed. Therefore
it does not delete it. But it does not call FreeLibrary yet.
* Emacs1 renames foo.eln to foo.eln.old. It tries to delete it but fails since
Emacs2 has an open handle.
* Emacs2 finally calls FreeLibrary() and closes.
In this case we are left over with a stale foo.eln.old. I don't think we can
have a race free algorithm.
We could add a "GC" step to `load`, where it tries to find stale .eln.old
files and removes them.
Nicolas.
El jue., 14 may. 2020 a las 15:13, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> >> 1- eln files do not retain the orginal function bytecode.
> >
> > That should be easy to change.
>
> Sure but we don't want to do that because there's no reason to bloat the
> eln.
>
> >> in any point of the code you can get the symbol-function of a defined
> > function and leak it everywhere.
> >
> > This is why I said the GC should do it. It should be able to find all
> > references.
>
> Yes but I can't *remove* objects that are referenced niether swap them.
> See below.
>
> >> We can't technically "swap" functions definitions, the best we do is just
> >> redefine them that is set a certain symbol-function (what late load does).
> >
> > When the GC finds a Lisp_Object that points to a subr we want to unload,
> > it should replace it with one that points to its bytecode equivalent version.
>
> No it cannot, if an object has been tested to say satisfy a predicate
> you cannot change it for another one. It would break tons of code and
> make the system totally un-debuggable.
>
> >> Even worst the function you want to remove could be active in the stack!
> >
> > You are right. I hadn't considered this. It can still be done, but would need to
> > wait until the function finishes.
>
> Yes, but when are all functions you want to get rid deactivated?
>
> Here we are really complexifying a problem that is not. IMO renaming
> and having a list to do the clean-up are sufficient tools to solve it.
>
> --
> akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 18:50:02 GMT)
Full text and
rfc822 format available.
Message #101 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Here we are really complexifying a problem that is not. IMO renaming
>> and having a list to do the clean-up are sufficient tools to solve it.
>
> You're right. I just think that renaming and a list do not guarantee that we
> always delete the files when we should.
>
> Consider this case:
>
> * Emacs1 and Emacs2 are two Emacs instances that load the
> same foo.eln file.
>
> * Emacs1 decides to recompile foo.el. To do that it renames foo.eln to
> foo.eln.old. It creates a new foo.eln and tries to delete foo.eln.old. It
> fails because Emacs2 has an open handle to it.
>
> * When Emacs2 closes it realizes that foo.eln has been renamed to foo.eln.old
> and deletes it.
>
> That is the good case.
>
> If we are unlucky this is what may happen:
>
> * Emacs2 begins to close. It checks that foo.eln has not been renamed. Therefore
> it does not delete it. But it does not call FreeLibrary yet.
>
> * Emacs1 renames foo.eln to foo.eln.old. It tries to delete it but fails since
> Emacs2 has an open handle.
>
> * Emacs2 finally calls FreeLibrary() and closes.
>
> In this case we are left over with a stale foo.eln.old. I don't think we can
> have a race free algorithm.
>
> We could add a "GC" step to `load`, where it tries to find stale .eln.old
> files and removes them.
>
> Nicolas.
I see. But I suspect it could work just if each Emacs sessions depose a
file to signal is activelly using a certain .eln.
The last session can retrive the current filename of the handle and
delete it.
Do you think it works?
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:00:02 GMT)
Full text and
rfc822 format available.
Message #104 received at submit <at> debbugs.gnu.org (full text, mbox):
Andrea Corallo writes:
> Mmmhh, probably also a lock file should be deposed by each Emacs using
> the file? That way the last session closing could check and remove?
You might be interested in this:
https://archive.fosdem.org/2018/schedule/speaker/michael_haubenwallner/
(Gentoo PRefix running on Cygwin, which means that in-use libraries need
to be replaceable.)
Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+
Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:01:02 GMT)
Full text and
rfc822 format available.
Message #107 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Do you think it works?
I don't know. What do you have in mind?
Another option:
We could use a global mutex shared between all Emacs processes.
https://docs.microsoft.com/en-us/windows/win32/sync/mutex-objects
They would have to acquire that mutex before any operation using eln files.
This is a complicated solution, though.
Nicolás.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:01:02 GMT)
Full text and
rfc822 format available.
Message #110 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 15:40:28 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> * Emacs1 and Emacs2 are two Emacs instances that load the
> same foo.eln file.
>
> * Emacs1 decides to recompile foo.el. To do that it renames foo.eln to
> foo.eln.old. It creates a new foo.eln and tries to delete foo.eln.old. It
> fails because Emacs2 has an open handle to it.
>
> * When Emacs2 closes it realizes that foo.eln has been renamed to foo.eln.old
> and deletes it.
How can Emacs2 realize that foo.eln was renamed?
> If we are unlucky this is what may happen:
>
> * Emacs2 begins to close. It checks that foo.eln has not been renamed. Therefore
> it does not delete it. But it does not call FreeLibrary yet.
>
> * Emacs1 renames foo.eln to foo.eln.old. It tries to delete it but fails since
> Emacs2 has an open handle.
>
> * Emacs2 finally calls FreeLibrary() and closes.
>
> In this case we are left over with a stale foo.eln.old. I don't think we can
> have a race free algorithm.
We will have to tell users not to do that on Windows.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:14:08 GMT)
Full text and
rfc822 format available.
Message #113 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 15:40:28 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> * Emacs1 renames foo.eln to foo.eln.old. It tries to delete it but fails since
> Emacs2 has an open handle.
>
> * Emacs2 finally calls FreeLibrary() and closes.
>
> In this case we are left over with a stale foo.eln.old. I don't think we can
> have a race free algorithm.
When Emacs1 fails to delete foo.eln.old, it could record the file's
deletion to be attempted again when it exits. That won't solve all
the cases, but it will solve some of them.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:16:01 GMT)
Full text and
rfc822 format available.
Message #116 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Do you think it works?
>
> I don't know. What do you have in mind?
Not much more than that.
As you said the problem is to decide who has the duty to remove the file
at last. If each Emacs deposes a file says .foo.eln-pidxxx for the
whole time is using foo.eln should be easy for the last Emacs to
understand it is really the last and has to do the clean-up in the case
foo.eln was renamed in foo.eln*whatever
I think it could work. (?)
> Another option:
>
> We could use a global mutex shared between all Emacs processes.
> https://docs.microsoft.com/en-us/windows/win32/sync/mutex-objects
>
> They would have to acquire that mutex before any operation using eln files.
> This is a complicated solution, though.
>
> Nicolás.
>
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:18:02 GMT)
Full text and
rfc822 format available.
Message #119 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 16:00:02 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> We could use a global mutex shared between all Emacs processes.
> https://docs.microsoft.com/en-us/windows/win32/sync/mutex-objects
>
> They would have to acquire that mutex before any operation using eln files.
> This is a complicated solution, though.
If we want a mutex, we can use the offending .eln file as that mutex:
if deletion fails, the mutex is taken.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:38:01 GMT)
Full text and
rfc822 format available.
Message #122 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> How can Emacs2 realize that foo.eln was renamed?
Using GetModuleFileNameA
https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:49:01 GMT)
Full text and
rfc822 format available.
Message #125 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> As you said the problem is to decide who has the duty to remove the file
> at last. If each Emacs deposes a file says .foo.eln-pidxxx for the
> whole time is using foo.eln should be easy for the last Emacs to
> understand it is really the last and has to do the clean-up in the case
> foo.eln was renamed in foo.eln*whatever
That file would be create when opening foo.eln, but when
Emacs is closing we don't know what file it refers to:
- foo.eln
- foo.eln.old
- foo.eln.old2
- foo.eln.oldN
These last files could be created if foo.eln.old exists at the time of renaming.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 19:59:01 GMT)
Full text and
rfc822 format available.
Message #128 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> As you said the problem is to decide who has the duty to remove the file
>> at last. If each Emacs deposes a file says .foo.eln-pidxxx for the
>> whole time is using foo.eln should be easy for the last Emacs to
>> understand it is really the last and has to do the clean-up in the case
>> foo.eln was renamed in foo.eln*whatever
>
> That file would be create when opening foo.eln, but when
> Emacs is closing we don't know what file it refers to:
> - foo.eln
> - foo.eln.old
> - foo.eln.old2
> - foo.eln.oldN
>
> These last files could be created if foo.eln.old exists at the time of renaming.
Yes, but I think we could say: the last Emacs closing that used any file
that was (at a certain point in life) foo.eln removes all the old
foo.eln.*
If is the last using any of the foo.eln* it can do that safely no?
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 20:17:02 GMT)
Full text and
rfc822 format available.
Message #131 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Yes, but I think we could say: the last Emacs closing that used any file
> that was (at a certain point in life) foo.eln removes all the old
> foo.eln.*
I think this would work :).
We could even remove the pid file. Just do the equivalent of `rm $ELN.old*`
after FreeLibrary(). If the deletion fails then that means that another Emacs
has loaded that file. It would take of files left over from crashes too.
We would need to change `package-delete` though. It would no longer fully
delete the directory. Maybe other functions in `package.el` would need
to be updated to deal with these changes.
Nicolas.
El jue., 14 may. 2020 a las 16:58, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> >> As you said the problem is to decide who has the duty to remove the file
> >> at last. If each Emacs deposes a file says .foo.eln-pidxxx for the
> >> whole time is using foo.eln should be easy for the last Emacs to
> >> understand it is really the last and has to do the clean-up in the case
> >> foo.eln was renamed in foo.eln*whatever
> >
> > That file would be create when opening foo.eln, but when
> > Emacs is closing we don't know what file it refers to:
> > - foo.eln
> > - foo.eln.old
> > - foo.eln.old2
> > - foo.eln.oldN
> >
> > These last files could be created if foo.eln.old exists at the time of renaming.
>
> Yes, but I think we could say: the last Emacs closing that used any file
> that was (at a certain point in life) foo.eln removes all the old
> foo.eln.*
>
> If is the last using any of the foo.eln* it can do that safely no?
>
> --
> akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 20:30:02 GMT)
Full text and
rfc822 format available.
Message #134 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Yes, but I think we could say: the last Emacs closing that used any file
>> that was (at a certain point in life) foo.eln removes all the old
>> foo.eln.*
>
> I think this would work :).
Very good
> We could even remove the pid file. Just do the equivalent of `rm $ELN.old*`
> after FreeLibrary(). If the deletion fails then that means that another Emacs
> has loaded that file. It would take of files left over from crashes too.
Ah okay I thought (probably had to read better) something goes wrong if
you remove when you should not. Then is even easier yes!
> We would need to change `package-delete` though. It would no longer fully
> delete the directory. Maybe other functions in `package.el` would need
> to be updated to deal with these changes.
If you diff the full branch I had to adjust few thing in Emacs too to
have it working, I believe is expected. You'll check for the presence
of the native compiler in Lisp with the function you've introduced in
one of your patches.
I believe also that the renaming mechanism should be transparent on all
posix where is not necessary.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 14 May 2020 20:35:01 GMT)
Full text and
rfc822 format available.
Message #137 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> We would need to change `package-delete` though. It would no longer fully
> delete the directory. Maybe other functions in `package.el` would need
> to be updated to deal with these changes.
Maybe not. We could move the .eln to the parent of the argument to
`delete-directory`.
For example move
elpa\28.0\develop\company-20200510.1614\eln-x86_64-w64-mingw32-683c5a1b96c51930\company.eln
to
elpa\28.0\develop\company.eln.old
The code to find the .eln.old files to delete would have to check if
it is a package it is dealing with and search for .eln.old files in an
upper folder.
Nicolas.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 15 May 2020 06:09:01 GMT)
Full text and
rfc822 format available.
Message #140 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 16:36:49 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > How can Emacs2 realize that foo.eln was renamed?
>
> Using GetModuleFileNameA
> https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea
Does that reflect renaming _after_ the library was loaded?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 15 May 2020 06:11:02 GMT)
Full text and
rfc822 format available.
Message #143 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Thu, 14 May 2020 16:48:06 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> > As you said the problem is to decide who has the duty to remove the file
> > at last. If each Emacs deposes a file says .foo.eln-pidxxx for the
> > whole time is using foo.eln should be easy for the last Emacs to
> > understand it is really the last and has to do the clean-up in the case
> > foo.eln was renamed in foo.eln*whatever
>
> That file would be create when opening foo.eln, but when
> Emacs is closing we don't know what file it refers to:
> - foo.eln
> - foo.eln.old
> - foo.eln.old2
> - foo.eln.oldN
>
> These last files could be created if foo.eln.old exists at the time of renaming.
We could try deleting all of them. Those that are still in use will
not be deleted.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 15 May 2020 12:34:02 GMT)
Full text and
rfc822 format available.
Message #146 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Does that reflect renaming _after_ the library was loaded?
I get that idea from the documentation. I haven't actually tested it myself.
The ProcessExplorer utility can show the name of the loaded DLLs
of a process. If I rename one the changed name will show up in ProcessExplorer.
I wonder how they do it. Too bad we don't have its source code.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 15 May 2020 13:02:01 GMT)
Full text and
rfc822 format available.
Message #149 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Fri, 15 May 2020 09:33:32 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > Does that reflect renaming _after_ the library was loaded?
>
> I get that idea from the documentation. I haven't actually tested it myself.
It should be easy to write a program to test this.
> The ProcessExplorer utility can show the name of the loaded DLLs
> of a process. If I rename one the changed name will show up in ProcessExplorer.
> I wonder how they do it.
They use methods that are generally unacceptable in general-purpose
applications, including injecting code into other processes, using
undocumented features and APIs, etc.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 15 May 2020 19:45:02 GMT)
Full text and
rfc822 format available.
Message #152 received at 41242 <at> debbugs.gnu.org (full text, mbox):
To summarize:
The best idea seems to be to rename the .eln file when removing the package or
recompiling. We need to reach a consensus on where to put the old .eln file,
though.
There are two options:
- Put it in the same folder as the original .eln file. This means that
`package-delete` will not be able to delete the directory. Additionally, it
will be tricky to handle left over files from an instance of Emacs that
crashes.
- Another option is to move them to `package-user-dir`. This option means that
`package-delete` will be able to delete the directory.
What option do you prefer?
The implementation I have in mind is roughly like this:
- `package-delete` iterates over the .eln files in the package directory. It
tries to delete it, if it fails it is moved to somewhere (see point above).
- When Emacs GCs a native compilation unit it should check if it has been
renamed (need to check if GetModuleFileNameA is fit for this). If it has, it
tries to delete it. If it fails, then some other Emacs instance must be using
it.
- The last step before calling exit() should FreeLibrary() all remaining .eln
files and run the equivalent of `rm $package_user_dir/*.eln.old`.
I think this would work and should be simple to implement. If I get your OK I'll
try to do it this weekend.
Nicolas
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 06:23:02 GMT)
Full text and
rfc822 format available.
Message #155 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Fri, 15 May 2020 16:44:04 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> To summarize:
>
> The best idea seems to be to rename the .eln file when removing the package or
> recompiling. We need to reach a consensus on where to put the old .eln file,
> though.
>
> There are two options:
>
> - Put it in the same folder as the original .eln file. This means that
> `package-delete` will not be able to delete the directory. Additionally, it
> will be tricky to handle left over files from an instance of Emacs that
> crashes.
>
> - Another option is to move them to `package-user-dir`. This option means that
> `package-delete` will be able to delete the directory.
>
> What option do you prefer?
I'm not sure I understand why we are talking only about package.el.
Wouldn't the same problem happen if the user recompiles a .el file for
which a .eln file already exists and is loaded into the session? And
similarly when Emacs is being built and all the *.el files are being
compiled or recompiled, sometimes by several Emacs processes running
in parallel via "make -jN"?
I think the solution should handle all of these use cases, not just
that of package.el upgrading a package. Do you agree?
> - `package-delete` iterates over the .eln files in the package directory. It
> tries to delete it, if it fails it is moved to somewhere (see point above).
>
> - When Emacs GCs a native compilation unit it should check if it has been
> renamed (need to check if GetModuleFileNameA is fit for this). If it has, it
> tries to delete it. If it fails, then some other Emacs instance must be using
> it.
>
> - The last step before calling exit() should FreeLibrary() all remaining .eln
> files and run the equivalent of `rm $package_user_dir/*.eln.old`.
Sounds OK to me, I don't think we came up with anything better during
the discussion till now.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 07:13:01 GMT)
Full text and
rfc822 format available.
Message #158 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
>> Date: Fri, 15 May 2020 16:44:04 -0300
>> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>>
>> To summarize:
>>
>> The best idea seems to be to rename the .eln file when removing the package or
>> recompiling. We need to reach a consensus on where to put the old .eln file,
>> though.
>>
>> There are two options:
>>
>> - Put it in the same folder as the original .eln file. This means that
>> `package-delete` will not be able to delete the directory. Additionally, it
>> will be tricky to handle left over files from an instance of Emacs that
>> crashes.
>>
>> - Another option is to move them to `package-user-dir`. This option means that
>> `package-delete` will be able to delete the directory.
>>
>> What option do you prefer?
>
> I'm not sure I understand why we are talking only about package.el.
> Wouldn't the same problem happen if the user recompiles a .el file for
> which a .eln file already exists and is loaded into the session?
IIUC the complication of package.el that makes it different from the
general cases you have described is that the eln-... sub-directory
has to be removed to be able to clean-up entirely the package.
> And
> similarly when Emacs is being built and all the *.el files are being
> compiled or recompiled, sometimes by several Emacs processes running
> in parallel via "make -jN"?
>
> I think the solution should handle all of these use cases, not just
> that of package.el upgrading a package. Do you agree?
>
>> - `package-delete` iterates over the .eln files in the package directory. It
>> tries to delete it, if it fails it is moved to somewhere (see point above).
>>
>> - When Emacs GCs a native compilation unit it should check if it has been
>> renamed (need to check if GetModuleFileNameA is fit for this). If it has, it
>> tries to delete it. If it fails, then some other Emacs instance must be using
>> it.
>>
>> - The last step before calling exit() should FreeLibrary() all remaining .eln
>> files and run the equivalent of `rm $package_user_dir/*.eln.old`.
>
> Sounds OK to me, I don't think we came up with anything better during
> the discussion till now.
>
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 16:13:01 GMT)
Full text and
rfc822 format available.
Message #161 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I'm not sure I understand why we are talking only about package.el.
> Wouldn't the same problem happen if the user recompiles a .el file for
> which a .eln file already exists and is loaded into the session?
True, right now the .eln would not be removed. Not even in Posix.
Therefore it will continue to be loaded unless `load-prefer-newer` is true.
> And
> similarly when Emacs is being built and all the *.el files are being
> compiled or recompiled, sometimes by several Emacs processes running
> in parallel via "make -jN"
Help me understand. Are you referring to the case where a developer changes
an .el file for which an .eln file (now outdated) already exists? I think
fixing the
case above will fix this one.
El sáb., 16 may. 2020 a las 3:22, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Fri, 15 May 2020 16:44:04 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > To summarize:
> >
> > The best idea seems to be to rename the .eln file when removing the
> package or
> > recompiling. We need to reach a consensus on where to put the old .eln
> file,
> > though.
> >
> > There are two options:
> >
> > - Put it in the same folder as the original .eln file. This means that
> > `package-delete` will not be able to delete the directory.
> Additionally, it
> > will be tricky to handle left over files from an instance of Emacs that
> > crashes.
> >
> > - Another option is to move them to `package-user-dir`. This option
> means that
> > `package-delete` will be able to delete the directory.
> >
> > What option do you prefer?
>
> I'm not sure I understand why we are talking only about package.el.
> Wouldn't the same problem happen if the user recompiles a .el file for
> which a .eln file already exists and is loaded into the session? And
> similarly when Emacs is being built and all the *.el files are being
> compiled or recompiled, sometimes by several Emacs processes running
> in parallel via "make -jN"?
>
> I think the solution should handle all of these use cases, not just
> that of package.el upgrading a package. Do you agree?
>
> > - `package-delete` iterates over the .eln files in the package
> directory. It
> > tries to delete it, if it fails it is moved to somewhere (see point
> above).
> >
> > - When Emacs GCs a native compilation unit it should check if it has been
> > renamed (need to check if GetModuleFileNameA is fit for this). If it
> has, it
> > tries to delete it. If it fails, then some other Emacs instance must
> be using
> > it.
> >
> > - The last step before calling exit() should FreeLibrary() all remaining
> .eln
> > files and run the equivalent of `rm $package_user_dir/*.eln.old`.
>
> Sounds OK to me, I don't think we came up with anything better during
> the discussion till now.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 16:20:02 GMT)
Full text and
rfc822 format available.
Message #164 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 16 May 2020 13:12:20 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > I'm not sure I understand why we are talking only about package.el.
> > Wouldn't the same problem happen if the user recompiles a .el file for
> > which a .eln file already exists and is loaded into the session?
>
> True, right now the .eln would not be removed. Not even in Posix.
No, on Posix systems we can delete the file, and it will be actually
deleted when its last handle is closed. I believe this works with
shared libraries as well.
> > And
> > similarly when Emacs is being built and all the *.el files are being
> > compiled or recompiled, sometimes by several Emacs processes running
> > in parallel via "make -jN"
>
> Help me understand. Are you referring to the case where a developer changes
> an .el file for which an .eln file (now outdated) already exists?
No, I mean building Emacs with "make -j10 bootstrap".
> I think fixing the case above will fix this one.
It's the same problem, yes. Just a slightly different use case, which
could therefore have different probabilities for some aspects. For
example, the probability of the same .el file being recompiled from
two separate sessions is relatively small, except when you consider
the "make -jN" use case.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 16:32:01 GMT)
Full text and
rfc822 format available.
Message #167 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> No, on Posix systems we can delete the file, and it will be actually
> deleted when its last handle is closed. I believe this works with
> shared libraries as well.
Do we actually do it? I don't think so. I don't even know where exactly
that check should be. Maybe `eval-buffer`?
> It's the same problem, yes. Just a slightly different use case, which
> could therefore have different probabilities for some aspects. For
> example, the probability of the same .el file being recompiled from
> two separate sessions is relatively small, except when you consider
> the "make -jN" use case.
The probability of two Emacs recompiling the same file in the "make -jN"
case is
0. That is because the build system tells each Emacs to compile one and
only one
file that is passed as a parameter. See #41329 for a bug related to this.
The case of two Emacs sessions recompiling the same file at the same time is
actually a problem.
We could implement the following algorithm:
- Have libgccjit write the .eln to a temporary name in the destination
folder.
- While "foo.eln" exists:
- Rename it to "foo.eln.oldN".
- Move the new .eln file to "foo.eln"
Nicolas
El sáb., 16 may. 2020 a las 13:19, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Sat, 16 May 2020 13:12:20 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > > I'm not sure I understand why we are talking only about package.el.
> > > Wouldn't the same problem happen if the user recompiles a .el file for
> > > which a .eln file already exists and is loaded into the session?
> >
> > True, right now the .eln would not be removed. Not even in Posix.
>
> No, on Posix systems we can delete the file, and it will be actually
> deleted when its last handle is closed. I believe this works with
> shared libraries as well.
>
> > > And
> > > similarly when Emacs is being built and all the *.el files are being
> > > compiled or recompiled, sometimes by several Emacs processes running
> > > in parallel via "make -jN"
> >
> > Help me understand. Are you referring to the case where a developer
> changes
> > an .el file for which an .eln file (now outdated) already exists?
>
> No, I mean building Emacs with "make -j10 bootstrap".
>
> > I think fixing the case above will fix this one.
>
> It's the same problem, yes. Just a slightly different use case, which
> could therefore have different probabilities for some aspects. For
> example, the probability of the same .el file being recompiled from
> two separate sessions is relatively small, except when you consider
> the "make -jN" use case.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 16:43:01 GMT)
Full text and
rfc822 format available.
Message #170 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 16 May 2020 13:31:41 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > No, on Posix systems we can delete the file, and it will be actually
> > deleted when its last handle is closed. I believe this works with
> > shared libraries as well.
>
> Do we actually do it? I don't think so. I don't even know where exactly
> that check should be. Maybe `eval-buffer`?
I'm not sure I understand what check did you have in mind.
> > It's the same problem, yes. Just a slightly different use case, which
> > could therefore have different probabilities for some aspects. For
> > example, the probability of the same .el file being recompiled from
> > two separate sessions is relatively small, except when you consider
> > the "make -jN" use case.
>
> The probability of two Emacs recompiling the same file in the "make -jN" case is
> 0.
Sorry, I meant the probability of one session compiling a file while
another uses it.
> The case of two Emacs sessions recompiling the same file at the same time is
> actually a problem.
>
> We could implement the following algorithm:
>
> - Have libgccjit write the .eln to a temporary name in the destination folder.
>
> - While "foo.eln" exists:
> - Rename it to "foo.eln.oldN".
> - Move the new .eln file to "foo.eln"
Something like that, yes.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 16 May 2020 17:11:02 GMT)
Full text and
rfc822 format available.
Message #173 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I'm not sure I understand what check did you have in mind.
I misunderstood your first message. I thought you were talking about
deleting
the .eln file when it no longer is up to date after the .el file changes.
If we want that we will have to add it to `auto-compile.el`.
El sáb., 16 may. 2020 a las 13:42, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Sat, 16 May 2020 13:31:41 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > > No, on Posix systems we can delete the file, and it will be actually
> > > deleted when its last handle is closed. I believe this works with
> > > shared libraries as well.
> >
> > Do we actually do it? I don't think so. I don't even know where exactly
> > that check should be. Maybe `eval-buffer`?
>
> I'm not sure I understand what check did you have in mind.
>
> > > It's the same problem, yes. Just a slightly different use case, which
> > > could therefore have different probabilities for some aspects. For
> > > example, the probability of the same .el file being recompiled from
> > > two separate sessions is relatively small, except when you consider
> > > the "make -jN" use case.
> >
> > The probability of two Emacs recompiling the same file in the "make -jN"
> case is
> > 0.
>
> Sorry, I meant the probability of one session compiling a file while
> another uses it.
>
> > The case of two Emacs sessions recompiling the same file at the same
> time is
> > actually a problem.
> >
> > We could implement the following algorithm:
> >
> > - Have libgccjit write the .eln to a temporary name in the destination
> folder.
> >
> > - While "foo.eln" exists:
> > - Rename it to "foo.eln.oldN".
> > - Move the new .eln file to "foo.eln"
>
> Something like that, yes.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 19 May 2020 19:24:02 GMT)
Full text and
rfc822 format available.
Message #176 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I have written the attached patch to handle the deletion
of native compilation units still in use. I have tested that it works well.
[Message part 2 (text/html, inline)]
[0001-Improve-handling-of-native-compilation-units-still-i.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 19 May 2020 19:27:01 GMT)
Full text and
rfc822 format available.
Message #179 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
The following two patches handle minor issues:
* Removal of the emacs_root_dir() hack.
* Determine the number of processors for native compilation.
[Message part 2 (text/html, inline)]
[0001-Windows-Use-NUMBER_OF_PROCESSORS-environment-variabl.patch (application/octet-stream, attachment)]
[0001-Determine-the-emacs-root-dir-only-when-necessary.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 15:30:02 GMT)
Full text and
rfc822 format available.
Message #182 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Tue, 19 May 2020 16:25:53 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> * lisp/emacs-lisp/comp.el (comp-effective-async-max-jobs): Use
> NUMBER_OF_PROCESSORS environment variable if system is Windows NT,
> "nproc" if it is in PATH or a default of 1.
This shouldn't be necessary: we already have a function to determine
the number of processors, see get_native_system_info in w32.c. If you
need the result exported to Lisp, we can define a new variable which
will be populated with the value.
> Subject: [PATCH] Determine the emacs root dir only when necessary.
>
> * src/fileio.c: Introduce function emacs_root_dir(). Refactor
> `expand-file-name` to use it.
> * src/lisp.h: Separate emacs_root_dir() into dos_emacs_root_dir() and
> w32_emacs_root_dir().
> * src/msdos.c: Rename emacs_root_dir() to dos_emacs_root_dir().
> * src/w32.c: Rename emacs_root_dir() to w32_emacs_root_dir().
Can you explain what problem this solves, and how? It is especially
important to understand when will be emacs_root_dir first called
during a session. That's because it calls filename_from_ansi, which
AFAIR needs some setup that happens at the beginning of a session.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 15:47:02 GMT)
Full text and
rfc822 format available.
Message #185 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> Can you explain what problem this solves, and how? It is especially
> important to understand when will be emacs_root_dir first called
> during a session. That's because it calls filename_from_ansi, which
> AFAIR needs some setup that happens at the beginning of a session.
When loading a dump file that contains native compiled elisp we try to find
the
.eln file. This uses Ffile_exists_p(). This function, in turn, calls
Fexpand_file_name(). This function will use the root directory as
`default_directory' as a fallback.
Getting the root directory requires reading the $emacs_dir environment
variable.
This is setup later in the initialization process. This caused a crash.
Fexpand_file_name() was trying to obtain the root directory even when it
was not
necessary because `default-directory' was not nil. My patch makes sure that
this
only happens when necessary.
It turns out that the dump loading process does not set `default-directory'
to
nil, therefore Fexpand_file_name() does not need to find out the root
directory
and we avoid reading an environment variable that is not set yet.
With this patch we avoid calling filename_from_ansi() too early (It is not
the
reason why Emacs crashed, but it is still a good idea to call it after it
has
been setup properly.
Nico
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 15:56:01 GMT)
Full text and
rfc822 format available.
Message #188 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Tue, 19 May 2020 16:23:06 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> I have written the attached patch to handle the deletion
> of native compilation units still in use. I have tested that it works well.
Thanks. I question the wisdom of handling the deletion from Lisp: if
this is specific to MS-Windows, then doing this from C would allow us
finer control on what happens. But this can be tackled later if
needed, so I'm not going to object to your code.
Your legal paperwork is done, so these changes can be installed on the
branch. Andrea, please review the changes, and feel free to push at
your discretion.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 16:07:02 GMT)
Full text and
rfc822 format available.
Message #191 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
>> Date: Tue, 19 May 2020 16:25:53 -0300
>> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>>
>> * lisp/emacs-lisp/comp.el (comp-effective-async-max-jobs): Use
>> NUMBER_OF_PROCESSORS environment variable if system is Windows NT,
>> "nproc" if it is in PATH or a default of 1.
>
> This shouldn't be necessary: we already have a function to determine
> the number of processors, see get_native_system_info in w32.c. If you
> need the result exported to Lisp, we can define a new variable which
> will be populated with the value.
>
>> Subject: [PATCH] Determine the emacs root dir only when necessary.
>>
>> * src/fileio.c: Introduce function emacs_root_dir(). Refactor
>> `expand-file-name` to use it.
>> * src/lisp.h: Separate emacs_root_dir() into dos_emacs_root_dir() and
>> w32_emacs_root_dir().
>> * src/msdos.c: Rename emacs_root_dir() to dos_emacs_root_dir().
>> * src/w32.c: Rename emacs_root_dir() to w32_emacs_root_dir().
>
> Can you explain what problem this solves, and how? It is especially
> important to understand when will be emacs_root_dir first called
> during a session. That's because it calls filename_from_ansi, which
> AFAIR needs some setup that happens at the beginning of a session.
I'm not into what is happening on Windows and why is working differently
than Posix, but I can add this as a context:
While we are restarting from dump to reload the native compilation unit
in dump_do_dump_relocation we are calling Ffile_exists_p
(pdumper.c:5307) and this is calling Fexpand_file_name.
We need to do that because we need the eln file to revive the
compilation unit object. Perhaps another way would be to use something
alternative to Ffile_exists_p?
But I've a question: is this a problem of Fexpand_file_name not working
correctly because something not initialized or a behavior we are trying
to change?
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 16:14:01 GMT)
Full text and
rfc822 format available.
Message #194 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Your legal paperwork is done, so these changes can be installed on the
> branch. Andrea, please review the changes, and feel free to push at
> your discretion.
Thanks, I'll start going through them this evening I guess in patch
chronological order.
Andrea
PS Nicolas if some patch is not up to date feel free to post the latest
version (or link to a public git repo if you prefer and have one).
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 16:19:01 GMT)
Full text and
rfc822 format available.
Message #197 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> Thanks, I'll start going through them this evening I guess in patch
> chronological order.
Thank you.
Here's the latest version.
[Message part 2 (text/html, inline)]
[0003-Handle-setjmp-taking-two-arguments-in-Windows.patch (application/octet-stream, attachment)]
[0004-Handle-LISP_WORDS_ARE_POINTERS-and-CHECK_LISP_OBJECT.patch (application/octet-stream, attachment)]
[0001-Determine-the-emacs-root-dir-only-when-necessary.patch (application/octet-stream, attachment)]
[0002-Do-not-block-SIGIO-in-platforms-that-don-t-have-it.patch (application/octet-stream, attachment)]
[0005-Remove-a-layer-of-indirection-for-access-to-pure-sto.patch (application/octet-stream, attachment)]
[0008-Windows-Use-NUMBER_OF_PROCESSORS-environment-variabl.patch (application/octet-stream, attachment)]
[0009-Improve-handling-of-native-compilation-units-still-i.patch (application/octet-stream, attachment)]
[0007-Load-libgccjit-dynamically-in-Windows.patch (application/octet-stream, attachment)]
[0006-Workaround-the-32768-chars-command-line-limit-in-Win.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 16:45:02 GMT)
Full text and
rfc822 format available.
Message #200 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Wed, 13 May 2020 16:26:57 -0300
>
> There are a few remaining issues:
>
> * The loading process is very slow. This is especially annoying when coupled
> with autoloading. For example, autoloading Helm may stall Emacs for 5 seconds
> in my machine.
>
> I have thought a possible solution to this problem: load the byte-compiled
> file and put the native-compiled version in a list. Then load that list one by
> one on an idle-timer, that way the UI freezes should be minimized. This could
> reuse the "late loading" machinery implemented for deferred compilation.
I believe we didn't investigate this issue well enough to understand
what happens and why. Would it be possible to investigate this soon?
It could be after the patches are installed.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 17:25:02 GMT)
Full text and
rfc822 format available.
Message #203 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Thanks, I'll start going through them this evening I guess in patch
>> chronological order.
Ok this is in:
68fad7a8fc @ Do not block SIGIO in platforms that don't have it.
Looking at 0003-Handle-setjmp-taking-two-arguments-in-Windows.patch
> +static void
> +define_setjmp_deps (void)
> +{
> + comp.setjmp_ctx_func
> + = gcc_jit_context_get_builtin_function (comp.ctxt,
> + "__builtin_frame_address");
> +}
This won't compile on Posix because setjmp_ctx_func is under #ifdef
_WIN32.
Given setjmp_ctx_func is used only in emit_setjmp I suggest to invoke
directly gcc_jit_context_get_builtin_function there while emitting the
corresponding call.
The following patches do not currently apply, please update them from
your rebase branch:
0004-Handle-LISP_WORDS_ARE_POINTERS-and-CHECK_LISP_OBJECT.patch
0005-Remove-a-layer-of-indirection-for-access-to-pure-sto.patch
While you are refreshing patches please have a look into the GNU style
of the diff (spaces in function calls and in pointer declaration) ;)
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 17:30:02 GMT)
Full text and
rfc822 format available.
Message #206 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
Sorry, still looking at
0003-Handle-setjmp-taking-two-arguments-in-Windows.patch forgot to ask
if you could comment on this change:
> comp.char_type,
> - sizeof (jmp_buf)),
> + sizeof (sys_jmp_buf)),
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 18:01:01 GMT)
Full text and
rfc822 format available.
Message #209 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
> Date: Wed, 20 May 2020 17:29:37 +0000
>
> Sorry, still looking at
> 0003-Handle-setjmp-taking-two-arguments-in-Windows.patch forgot to ask
> if you could comment on this change:
>
> > comp.char_type,
> > - sizeof (jmp_buf)),
> > + sizeof (sys_jmp_buf)),
I think the latter is more portable, see lisp.h around line 2150.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 18:10:02 GMT)
Full text and
rfc822 format available.
Message #212 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> I think the latter is more portable, see lisp.h around line 2150.
I see thanks.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 18:50:01 GMT)
Full text and
rfc822 format available.
Message #215 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> This won't compile on Posix because setjmp_ctx_func is under #ifdef
> _WIN32.
Fixed.
> Given setjmp_ctx_func is used only in emit_setjmp I suggest to invoke
> directly gcc_jit_context_get_builtin_function there while emitting the
> corresponding call.
Done.
> The following patches do not currently apply, please update them from
> your rebase branch:
> 0004-Handle-LISP_WORDS_ARE_POINTERS-and-CHECK_LISP_OBJECT.patch
> 0005-Remove-a-layer-of-indirection-for-access-to-pure-sto.patch
Done.
> While you are refreshing patches please have a look into the GNU style
> of the diff (spaces in function calls and in pointer declaration) ;)
Fixed :).
> Sorry, still looking at
> 0003-Handle-setjmp-taking-two-arguments-in-Windows.patch forgot to ask
> if you could comment on this change:
> > comp.char_type,
> > - sizeof (jmp_buf)),
> > + sizeof (sys_jmp_buf)),
It is just a little bit more portable.
I uploaded new patches to https://github.com/nicber/emacs branch
feature/native-comp. Please tell me I if you'd rather I posted patches here.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 20 May 2020 21:39:03 GMT)
Full text and
rfc822 format available.
Message #218 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> I uploaded new patches to https://github.com/nicber/emacs branch
> feature/native-comp. Please tell me I if you'd rather I posted
> patches here.
Git works for me thanks.
I've pushed
05b08f2644 * Handle setjmp() taking two arguments in Windows.
5ff2cbdb04 * Remove a layer of indirection for access to pure storage.
I took the freedom to do myself some minors to avoid a re-spin, you can
have a look.
I started testing Handle-LISP_WORDS_ARE_POINTERS-and-CHECK_LISP_OBJECT
because is sensitive. The space of the configurations is relatively big
and I want to cover the one I can here so I think I'll be done tomorrow.
Thanks
Andrea
PS are you cooking-up also libgccjit patches?
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 21 May 2020 01:59:02 GMT)
Full text and
rfc822 format available.
Message #221 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> PS are you cooking-up also libgccjit patches?
Yes, the C++ side is done.
I need help with the build system side of things.
* libiberty.a needs to be copied by hand between folders inside the build
directory.
* libgccjit.dll does not get created in the "bin" folder.
It is created as libgccjit.so.0.0.1 in the "lib" folder, so it needs to
copied and renamed.
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 21 May 2020 18:52:01 GMT)
Full text and
rfc822 format available.
Message #224 received at 41242 <at> debbugs.gnu.org (full text, mbox):
I keep on working on top of "Handle LISP_WORDS_ARE_POINTERS and
CHECK_LISP_OBJECT_TYPE".
The x86_64 works like a charm and compile time is not affected.
I did some fixing for i386 --enable-check-lisp-object-type, now I'm on
wide-int where we get ICE in GCC. Hope to sort out quickly but can't
guarantee.
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> PS are you cooking-up also libgccjit patches?
> Yes, the C++ side is done.
> I need help with the build system side of things.
> * libiberty.a needs to be copied by hand between folders inside the
> build directory.
> * libgccjit.dll does not get created in the "bin" folder.
> It is created as libgccjit.so.0.0.1 in the "lib" folder, so it
> needs to copied and renamed.
Yeah build system is always tricky, but I guess defining a new target to
rename and copy or modifying the existing one for the new name should
not be a problem no? If you need help the better place to ask and
discuss is jit <at> gcc.gnu.org tho.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 22 May 2020 21:24:01 GMT)
Full text and
rfc822 format available.
Message #227 received at 41242 <at> debbugs.gnu.org (full text, mbox):
I've installed "Handle LISP_WORDS_ARE_POINTERS..." plus some fixes
mostly unrelated.
We still ICE GCC (at least my 10 and trunk) on i386 at speed 2 with
--enable-check-lisp-object-type. This really looks like a libgccjit
bug. Tomorrow I'll try to look into it better and to progress into
applying patches.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 01:57:02 GMT)
Full text and
rfc822 format available.
Message #230 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>> I have thought a possible solution to this problem: load the
byte-compiled
>> file and put the native-compiled version in a list. Then load that
list one by
>> one on an idle-timer, that way the UI freezes should be minimized.
This could
>> reuse the "late loading" machinery implemented for deferred
compilation.
> I believe we didn't investigate this issue well enough to understand
> what happens and why. Would it be possible to investigate this soon?
> It could be after the patches are installed.
I profiled Emacs and found out that most of the time it was blocked on a
call to
wopen().
A non-native-comp build will try to open these files in each path in
`load-path':
- foo.dll.gz
- foo.dll.gz
- foo.elc.gz
- foo.elc
- foo.el.gz
- foo.el
The native-comp build with try to open these files instead:
- eln-$HASH/foo.eln
- eln-$HASH/foo.eln.gz
- eln-$HASH/foo.dll
- eln-$HASH/foo.dll.gz
- eln-$HASH/foo.elc
- eln-$HASH/foo.elc.gz
- eln-$HASH/foo.el
- eln-$HASH/foo.el.gz
- foo.eln
- foo.eln.gz
- foo.dll
- foo.dll.gz
- foo.elc
- foo.elc.gz
- foo.el
- foo.el.gz
This is a 166% percent increase in calls to wopen(). Moreover, it is trying
to
open some files that don't make sense (*.dll.gz and *.eln.gz) and it's
testing
the eln-$HASH directory for *.el and *.elc files.
This is what is should be probing:
- eln-$HASH/foo.eln (only native-comp)
- foo.eln (only native-comp)
- foo.dll
- foo.elc
- foo.elc.gz
- foo.el
- foo.el.gz
This is "only" a 40% increase. I don't think we can do much better here.
That
is, unless we implement some kind of cache, but that would be an extreme
solution.
This problem shows in Windows because the NT kernel is slow at file access
(See
https://github.com/Microsoft/WSL/issues/873#issuecomment-425272829).
The same thing happens to Git for Windows in my experience.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 04:42:01 GMT)
Full text and
rfc822 format available.
Message #233 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> This is what is should be probing:
> - eln-$HASH/foo.eln (only native-comp)
> - foo.eln (only native-comp)
> - foo.dll
> - foo.elc
> - foo.elc.gz
> - foo.el
> - foo.el.gz
I did a quick test and make openp() only call emacs_open() when a file
matches one in the list above.
This cut Emacs startup time by half! This seems promising.
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 08:08:01 GMT)
Full text and
rfc822 format available.
Message #236 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Wed, 20 May 2020 12:46:32 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> When loading a dump file that contains native compiled elisp we try to find the
> .eln file. This uses Ffile_exists_p(). This function, in turn, calls
> Fexpand_file_name(). This function will use the root directory as
> `default_directory' as a fallback.
>
> Getting the root directory requires reading the $emacs_dir environment variable.
> This is setup later in the initialization process. This caused a crash.
Can you tell why this doesn't happen when *.elc files are used
instead? Also, what do you mean by "loading a dump file that contains
native compiled elisp"? how did native compiled ELisp get into the
dump file in the first place?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 08:36:02 GMT)
Full text and
rfc822 format available.
Message #239 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
>> Date: Wed, 20 May 2020 12:46:32 -0300
>> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>>
>> When loading a dump file that contains native compiled elisp we try to find the
>> .eln file. This uses Ffile_exists_p(). This function, in turn, calls
>> Fexpand_file_name(). This function will use the root directory as
>> `default_directory' as a fallback.
>>
>> Getting the root directory requires reading the $emacs_dir environment variable.
>> This is setup later in the initialization process. This caused a crash.
>
> Can you tell why this doesn't happen when *.elc files are used
> instead? Also, what do you mean by "loading a dump file that contains
> native compiled elisp"? how did native compiled ELisp get into the
> dump file in the first place?
Hi Eli,
you may be interested in this paragraph
https://akrl.sdf.org/gccemacs.html#orgb063106
Regards
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 09:23:02 GMT)
Full text and
rfc822 format available.
Message #242 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: Nicolas Bértolo <nicolasbertolo <at> gmail.com>,
> 41242 <at> debbugs.gnu.org
> Date: Sat, 23 May 2020 08:35:43 +0000
>
> >> When loading a dump file that contains native compiled elisp we try to find the
> >> .eln file. This uses Ffile_exists_p(). This function, in turn, calls
> >> Fexpand_file_name(). This function will use the root directory as
> >> `default_directory' as a fallback.
> >>
> >> Getting the root directory requires reading the $emacs_dir environment variable.
> >> This is setup later in the initialization process. This caused a crash.
> >
> > Can you tell why this doesn't happen when *.elc files are used
> > instead? Also, what do you mean by "loading a dump file that contains
> > native compiled elisp"? how did native compiled ELisp get into the
> > dump file in the first place?
>
> Hi Eli,
>
> you may be interested in this paragraph
>
> https://akrl.sdf.org/gccemacs.html#orgb063106
Thanks. I guess this changes a lot in how Emacs starts up? E.g.,
what happens if the .el file was in the meantime modified and
recompiled into the .eln file? we will load the new versions instead
of the one that was preloaded, right?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 10:38:01 GMT)
Full text and
rfc822 format available.
Message #245 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Andrea Corallo <akrl <at> sdf.org>
>> Cc: Nicolas Bértolo <nicolasbertolo <at> gmail.com>,
>> 41242 <at> debbugs.gnu.org
>> Date: Sat, 23 May 2020 08:35:43 +0000
>>
>> >> When loading a dump file that contains native compiled elisp we try to find the
>> >> .eln file. This uses Ffile_exists_p(). This function, in turn, calls
>> >> Fexpand_file_name(). This function will use the root directory as
>> >> `default_directory' as a fallback.
>> >>
>> >> Getting the root directory requires reading the $emacs_dir environment variable.
>> >> This is setup later in the initialization process. This caused a crash.
>> >
>> > Can you tell why this doesn't happen when *.elc files are used
>> > instead? Also, what do you mean by "loading a dump file that contains
>> > native compiled elisp"? how did native compiled ELisp get into the
>> > dump file in the first place?
>>
>> Hi Eli,
>>
>> you may be interested in this paragraph
>>
>> https://akrl.sdf.org/gccemacs.html#orgb063106
>
> Thanks. I guess this changes a lot in how Emacs starts up? E.g.,
> what happens if the .el file was in the meantime modified and
> recompiled into the .eln file? we will load the new versions instead
> of the one that was preloaded, right?
This is something we have to define. Currently we only complain if one
of the defined functions that was dumped cannot be found in the new
.eln. My preference would be to sign each .eln used for dump to make
sure what we are loading is what we dumped and refuse to load otherwise.
BTW reloading from dump the "dumped" eln are not located by searching in
the load-path for all suffix. The eln position is stored into the
compilation unit object for performance reasons. The performance
problem Nicolas is discussing is related to the non dumped .elns loaded
at startup I believe.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 11:04:01 GMT)
Full text and
rfc822 format available.
Message #248 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: nicolasbertolo <at> gmail.com, 41242 <at> debbugs.gnu.org
> Date: Sat, 23 May 2020 10:37:18 +0000
>
> >> https://akrl.sdf.org/gccemacs.html#orgb063106
> >
> > Thanks. I guess this changes a lot in how Emacs starts up? E.g.,
> > what happens if the .el file was in the meantime modified and
> > recompiled into the .eln file? we will load the new versions instead
> > of the one that was preloaded, right?
>
> This is something we have to define. Currently we only complain if one
> of the defined functions that was dumped cannot be found in the new
> .eln. My preference would be to sign each .eln used for dump to make
> sure what we are loading is what we dumped and refuse to load otherwise.
>
> BTW reloading from dump the "dumped" eln are not located by searching in
> the load-path for all suffix. The eln position is stored into the
> compilation unit object for performance reasons.
Are you saying we store the absolute file name of the .eln files in
the pdumper file? If so, how can Emacs start up if the .eln files
were moved to another location, e.g. as part of "make install", or
more generally if Emacs was relocated since it was dumped?
> The performance problem Nicolas is discussing is related to the non
> dumped .elns loaded at startup I believe.
This is not about the performance issue, this is about the crash
because emacs_dir was not yet defined where Emacs needed it: a
different issue uncovered by Nicolas.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 11:22:02 GMT)
Full text and
rfc822 format available.
Message #251 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Andrea Corallo <akrl <at> sdf.org>
>> Cc: nicolasbertolo <at> gmail.com, 41242 <at> debbugs.gnu.org
>> Date: Sat, 23 May 2020 10:37:18 +0000
>>
>> >> https://akrl.sdf.org/gccemacs.html#orgb063106
>> >
>> > Thanks. I guess this changes a lot in how Emacs starts up? E.g.,
>> > what happens if the .el file was in the meantime modified and
>> > recompiled into the .eln file? we will load the new versions instead
>> > of the one that was preloaded, right?
>>
>> This is something we have to define. Currently we only complain if one
>> of the defined functions that was dumped cannot be found in the new
>> .eln. My preference would be to sign each .eln used for dump to make
>> sure what we are loading is what we dumped and refuse to load otherwise.
>>
>> BTW reloading from dump the "dumped" eln are not located by searching in
>> the load-path for all suffix. The eln position is stored into the
>> compilation unit object for performance reasons.
>
> Are you saying we store the absolute file name of the .eln files in
> the pdumper file? If so, how can Emacs start up if the .eln files
> were moved to another location, e.g. as part of "make install", or
> more generally if Emacs was relocated since it was dumped?
To be precise we store the relative path of the .eln from the emacs
executable, both for the local build both for the file position we will
have after a "make install".
Reloading the first compilation unit the code detect in which of this
two cases we are (this is where file-exists is called) and this
information is used for all the following compilaiton unit to be
revived.
>> The performance problem Nicolas is discussing is related to the non
>> dumped .elns loaded at startup I believe.
>
> This is not about the performance issue, this is about the crash
> because emacs_dir was not yet defined where Emacs needed it: a
> different issue uncovered by Nicolas.
Ops got confused between the two branches of this same thread.
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 12:21:02 GMT)
Full text and
rfc822 format available.
Message #254 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: nicolasbertolo <at> gmail.com, 41242 <at> debbugs.gnu.org
> Date: Sat, 23 May 2020 11:21:03 +0000
>
> >> BTW reloading from dump the "dumped" eln are not located by searching in
> >> the load-path for all suffix. The eln position is stored into the
> >> compilation unit object for performance reasons.
> >
> > Are you saying we store the absolute file name of the .eln files in
> > the pdumper file? If so, how can Emacs start up if the .eln files
> > were moved to another location, e.g. as part of "make install", or
> > more generally if Emacs was relocated since it was dumped?
>
> To be precise we store the relative path of the .eln from the emacs
> executable, both for the local build both for the file position we will
> have after a "make install".
>
> Reloading the first compilation unit the code detect in which of this
> two cases we are (this is where file-exists is called) and this
> information is used for all the following compilaiton unit to be
> revived.
Do we indeed have only 2 use cases regarding the relative file name of
.eln files, and not more?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 12:55:02 GMT)
Full text and
rfc822 format available.
Message #257 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> To be precise we store the relative path of the .eln from the emacs
>> executable, both for the local build both for the file position we will
>> have after a "make install".
>>
>> Reloading the first compilation unit the code detect in which of this
>> two cases we are (this is where file-exists is called) and this
>> information is used for all the following compilaiton unit to be
>> revived.
>
> Do we indeed have only 2 use cases regarding the relative file name of
> .eln files, and not more?
If is a question regarding the current implementation of the branch the
answer is yes.
If is a more generic question then the answer is AFAIK yes, but we could
add others if they are known at compile time and needed.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 14:42:01 GMT)
Full text and
rfc822 format available.
Message #260 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> This is something we have to define. Currently we only complain if one
> of the defined functions that was dumped cannot be found in the new
> .eln. My preference would be to sign each .eln used for dump to make
> sure what we are loading is what we dumped and refuse to load otherwise.
What if we find out where the linker has put the shared library, copy that
region
of memory into the dump file and when loading Emacs we mmap that data into
same address it was?
It is essentially saving the result of the linker for later use. This would
require
no ASLR and doing it ASAP to prevent the something from using that address
space.
Another option: statically link the .eln files (we'd need libgccjit to
create static libraries)
into the final Emacs executable. This would take care of function
definitions and
loading the dump would take care of the rest.
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 15:12:01 GMT)
Full text and
rfc822 format available.
Message #263 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> This is something we have to define. Currently we only complain if
> one
>> of the defined functions that was dumped cannot be found in the new
>> .eln. My preference would be to sign each .eln used for dump to
> make
>> sure what we are loading is what we dumped and refuse to load
> otherwise.
>
> What if we find out where the linker has put the shared library, copy
> that region
> of memory into the dump file and when loading Emacs we mmap that data
> into
> same address it was?
> It is essentially saving the result of the linker for later use. This
> would require
> no ASLR and doing it ASAP to prevent the something from using that
> address space.
I don't think would fly: You are not garanteed to be able to obtain the
same mmaped address anyway and we cannot go for a solution that does not
support ASLR. In general to be portable it cannot rely on assumptions
or low level tricks. I think these are (at least part of) the reasons
why we moved away from unexec.
> Another option: statically link the .eln files (we'd need libgccjit
> to create static libraries)
> into the final Emacs executable. This would take care of function
> definitions and
> loading the dump would take care of the rest.
Is not that simple, loading eln is mutating the environment with side
effects, function definition is just a part of that.
Even more important we must support subsequent dumps.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 15:28:01 GMT)
Full text and
rfc822 format available.
Message #266 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I don't think would fly: You are not garanteed to be able to obtain the
> same mmaped address anyway and we cannot go for a solution that does not
> support ASLR. In general to be portable it cannot rely on assumptions
> or low level tricks. I think these are (at least part of) the reasons
> why we moved away from unexec.
AFAIU ASLR is disabled already, at least in Windows.
> Is not that simple, loading eln is mutating the environment with side
> effects, function definition is just a part of that.
I know, but linking against a static .eln would just make the symbols
available,
not anything else. The mutation of the environment would happen when loading
the dump.
> Even more important we must support subsequent dumps.
You are right. I hadn't considered this.
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 15:53:02 GMT)
Full text and
rfc822 format available.
Message #269 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 11:41:19 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> Another option: statically link the .eln files (we'd need libgccjit to create static libraries)
> into the final Emacs executable. This would take care of function definitions and
> loading the dump would take care of the rest.
That would preclude re-dumping Emacs, no?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 16:01:01 GMT)
Full text and
rfc822 format available.
Message #272 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> I don't think would fly: You are not garanteed to be able to obtain
> the
>> same mmaped address anyway and we cannot go for a solution that
> does not
>> support ASLR. In general to be portable it cannot rely on
> assumptions
>> or low level tricks. I think these are (at least part of) the
> reasons
>> why we moved away from unexec.
>
> AFAIU ASLR is disabled already, at least in Windows.
The fact that is now disabled does not imply that we want to prevent us
from using it for the future, that said AFAIU given the personality of
my running Emacs is zeroed (i'm on GNU/Linux) it is running with ASLR
enabled. See also 'maybe_disable_address_randomization'.
>> Is not that simple, loading eln is mutating the environment with
> side
>> effects, function definition is just a part of that.
>
> I know, but linking against a static .eln would just make the symbols
> available,
> not anything else. The mutation of the environment would happen when
> loading
> the dump.
Is more complex because we can have in the eln code that at top level
relies on functions as:
(defun foo ()
..)
(some code that depends on the definition of foo)
(defun foo ()
..)
So function definition and other top level executon is mixed. I think
this could be worked around but is tricky.
>> Even more important we must support subsequent dumps.
>
> You are right. I hadn't considered this.
Yeah I think this the real no go.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 16:04:02 GMT)
Full text and
rfc822 format available.
Message #275 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> That would preclude re-dumping Emacs, no?
I guess so. I didn't think about that option.
As Andrea said, this is a no go.
El sáb., 23 may. 2020 a las 12:52, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Sat, 23 May 2020 11:41:19 -0300
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
> >
> > Another option: statically link the .eln files (we'd need libgccjit to
> create static libraries)
> > into the final Emacs executable. This would take care of function
> definitions and
> > loading the dump would take care of the rest.
>
> That would preclude re-dumping Emacs, no?
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 16:05:01 GMT)
Full text and
rfc822 format available.
Message #278 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 12:26:56 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41242 <at> debbugs.gnu.org
>
> AFAIU ASLR is disabled already, at least in Windows.
I don't think so, I see it on modern versions of Windows all the time.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 16:21:02 GMT)
Full text and
rfc822 format available.
Message #281 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I don't think so, I see it on modern versions of Windows all the time.
Using ProcessExplorer this is what I see in my machine:
- emacs.exe is always loaded @ 0x40000000.
- .eln files do not have the ASLR flag enabled and "Image Base" == "Base".
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 17:05:02 GMT)
Full text and
rfc822 format available.
Message #284 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 13:20:06 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > I don't think so, I see it on modern versions of Windows all the time.
>
> Using ProcessExplorer this is what I see in my machine:
>
> - emacs.exe is always loaded @ 0x40000000.
> - .eln files do not have the ASLR flag enabled and "Image Base" == "Base".
My observation was based on the fact that addresses of the same
objects that I see in the debugger are different from session to
session on Windows 7 and Windows 10, but not on XP.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 17:21:02 GMT)
Full text and
rfc822 format available.
Message #287 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> My observation was based on the fact that addresses of the same
> objects that I see in the debugger are different from session to
> session on Windows 7 and Windows 10, but not on XP.
That would mean that the heap is randomized, right? But the code could
still be always loaded at the same address.
Coming back to the performance problem when loading: apart from reducing
the number of files probed, we could try parallelizing openp() using a
thread
pool. What do you think?
Nicolas
El sáb., 23 may. 2020 a las 14:04, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Sat, 23 May 2020 13:20:06 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > > I don't think so, I see it on modern versions of Windows all the time.
> >
> > Using ProcessExplorer this is what I see in my machine:
> >
> > - emacs.exe is always loaded @ 0x40000000.
> > - .eln files do not have the ASLR flag enabled and "Image Base" ==
> "Base".
>
> My observation was based on the fact that addresses of the same
> objects that I see in the debugger are different from session to
> session on Windows 7 and Windows 10, but not on XP.
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 17:36:02 GMT)
Full text and
rfc822 format available.
Message #290 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 14:20:09 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > My observation was based on the fact that addresses of the same
> > objects that I see in the debugger are different from session to
> > session on Windows 7 and Windows 10, but not on XP.
>
> That would mean that the heap is randomized, right? But the code could
> still be always loaded at the same address.
I don't remember if it was only the heap, or also addresses of other
areas. For example, I think the stack was also in a different area.
> Coming back to the performance problem when loading: apart from reducing
> the number of files probed, we could try parallelizing openp() using a thread
> pool. What do you think?
I'd start by reducing the number of probed files, and then I'd
benchmark the results and see if it's "good enough". Threads add
another dimension of complexity, so I'd only go there if we have a
very good reason.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 17:48:02 GMT)
Full text and
rfc822 format available.
Message #293 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I'd start by reducing the number of probed files, and then I'd
> benchmark the results and see if it's "good enough". Threads add
> another dimension of complexity, so I'd only go there if we have a
> very good reason.
Just to show some numbers:
I run a fairly heavy Spacemacs configuration.
Without any patch it takes 80 seconds to start.
Reducing the number of probed files takes this down to 40 seconds.
The VTune profiler tells me it is spending 80% of the time waiting for
openp().
I'll rewrite the patch to reduce the number of probed files.
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 17:57:01 GMT)
Full text and
rfc822 format available.
Message #296 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Coming back to the performance problem when loading: apart from reducing
>> the number of files probed, we could try parallelizing openp() using a thread
>> pool. What do you think?
>
> I'd start by reducing the number of probed files, and then I'd
> benchmark the results and see if it's "good enough". Threads add
> another dimension of complexity, so I'd only go there if we have a
> very good reason.
Agree, also I think before that would be necessary to prove that
parallelizing in user space let the kernel scale up in performance in
serving the syscalls. I'm not really sure about that and the observed
bottle neck is apparently there.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 18:22:02 GMT)
Full text and
rfc822 format available.
Message #299 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 14:47:03 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> Just to show some numbers:
> I run a fairly heavy Spacemacs configuration.
> Without any patch it takes 80 seconds to start.
> Reducing the number of probed files takes this down to 40 seconds.
>
> The VTune profiler tells me it is spending 80% of the time waiting for openp().
If it takes us 32 seconds to run openp, then how many files do we try
to probe? 32 sec is eternity!
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 18:30:02 GMT)
Full text and
rfc822 format available.
Message #302 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> If it takes us 32 seconds to run openp, then how many files do we try
> to probe? 32 sec is eternity!
`load-path' has 380 entries, and load-prefer-newer is true. That is why it
is so slow.
Loading foo.el from package foo requires probing 6 different extensions
(.eln, .dll, .elc.gz, .elc, .el, .el.gz) in 380 directories.
6*380 = 2280 calls to wopen() per call to `load` or `require`
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 18:38:02 GMT)
Full text and
rfc822 format available.
Message #305 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 23 May 2020 15:29:25 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>
> > If it takes us 32 seconds to run openp, then how many files do we try
> > to probe? 32 sec is eternity!
>
> `load-path' has 380 entries
Is this due to Spacemacs setup, or something else? Why would you have
so many directories in your load-path?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 18:44:01 GMT)
Full text and
rfc822 format available.
Message #308 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> Why would you have so many directories in your load-path?
Because I have many Spacemacs layers installed, each installs many packages
and each package has its own directory in `package-user-dir`.
El sáb., 23 may. 2020 a las 15:37, Eli Zaretskii (<eliz <at> gnu.org>) escribió:
> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> > Date: Sat, 23 May 2020 15:29:25 -0300
> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
> >
> > > If it takes us 32 seconds to run openp, then how many files do we try
> > > to probe? 32 sec is eternity!
> >
> > `load-path' has 380 entries
>
> Is this due to Spacemacs setup, or something else? Why would you have
> so many directories in your load-path?
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 22:53:02 GMT)
Full text and
rfc822 format available.
Message #311 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I have come up with this patch to reduce the number of files
probed when `load' is called..
El sáb., 23 may. 2020 a las 15:43, Nicolas Bértolo (<
nicolasbertolo <at> gmail.com>) escribió:
> > Why would you have so many directories in your load-path?
>
> Because I have many Spacemacs layers installed, each installs many packages
> and each package has its own directory in `package-user-dir`.
>
> El sáb., 23 may. 2020 a las 15:37, Eli Zaretskii (<eliz <at> gnu.org>)
> escribió:
>
>> > From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
>> > Date: Sat, 23 May 2020 15:29:25 -0300
>> > Cc: Andrea Corallo <akrl <at> sdf.org>, 41242 <at> debbugs.gnu.org
>> >
>> > > If it takes us 32 seconds to run openp, then how many files do we try
>> > > to probe? 32 sec is eternity!
>> >
>> > `load-path' has 380 entries
>>
>> Is this due to Spacemacs setup, or something else? Why would you have
>> so many directories in your load-path?
>>
>
[Message part 2 (text/html, inline)]
[0001-Reduce-the-number-of-files-probed-when-finding-a-lis.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 22:59:02 GMT)
Full text and
rfc822 format available.
Message #314 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Hi Nicolas,
following some comments on - Improve handling of native compilation
etc.
Please review all the GNU code-style of this diff. I've annotated some
to be fixed but there are quite a number more of fixes of the same kind
to be done.
> When closing emacs will inspect all directories from which it loaded
> native compilation units. If it finds a ".eln.old" file it will try to
> delete it, if it fails that means that another Emacs instance is using it.
>
> When compiling a file we rename the file that was in the output path
> in case it has been loaded into another Emacs instance.
>
> When deleting a package we move any ".eln" or ".eln.old" files in the
> package folder that we can't delete to `package-user-dir`. Emacs will
> check that directory when closing and delete them.
>
> * lisp/emacs-lisp/comp.el (comp--replace-output-file): Function called
> from C code to finish the compilation process. It performs renaming of
> the old file if necessary.
> * lisp/emacs-lisp/package.el (package--delete-directory): Function to
> delete a package directory. It moves native compilation units that it
> can't delete to `package-user-dir'.
> * src/alloc.c (cleanup_vector): Call dispose_comp_unit().
> (garbage_collect): Call finish_delayed_disposal_of_comp_units().
> * src/comp.c: Restore the signal mask using unwind-protect. Store
> loaded native compilation units in a hash table for disposal on
> close. Store filenames of native compilation units GC'd in a linked
> list to finish their disposal when the GC is over.
Please annotate in the changelog the new functions ex:
finish_delayed_disposal_of_comp_units, dispose_all_remaining_comp_units,
register_native_comp_unit are missing.
> * src/comp.h: Introduce cfile member in Lisp_Native_Comp_Unit.
> Add declarations of functions that: clean directories of unused native
> compilation units, handle disposal of native compilation units.
> * src/emacs.c (kill-emacs): Dispose all remaining compilation units
> right right before calling exit().
> * src/eval.c (internal_condition_case_3, internal_condition_case_4):
> Add functions.
> * src/lisp.h (internal_condition_case_3, internal_condition_case_4):
> Add functions.
> * src/pdumper.c (dump_do_dump_relocation): Set cfile to a copy of the
> Lisp string specifying the file path.
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 012baf2560..1fb4cd98c0 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -2183,6 +2183,31 @@ comp-hint-cons
>
> ;; Some entry point support code.
>
> +(defun comp--replace-output-file (outfile tmpfile)
> + "Replace OUTFILE with TMPFILE taking the necessary steps when
> +dealing with shared libraries that may be loaded into Emacs"
> + (cond ((eq 'windows-nt system-type)
> + (ignore-errors (delete-file outfile))
> + (let ((retry t))
> + (while retry
> + (setf retry nil)
> + (condition-case _
> + (progn
> + ;; outfile maybe recreated by another Emacs in
> + ;; between the following two rename-file calls
> + (if (file-exists-p outfile)
> + (rename-file outfile (make-temp-file-internal
> + (file-name-sans-extension outfile)
> + nil ".eln.old" nil)
Isn't better to just add .old? So we will have cases of foo.eln.old.old
instead of foo.eln.old.eln.old ?
> + t))
> + (rename-file tmpfile outfile nil))
> + (file-already-exists (setf retry t))))))
> + ;; Remove the old eln instead of copying the new one into it
> + ;; to get a new inode and prevent crashes in case the old one
> + ;; is currently loaded.
> + (t (delete-file outfile)
> + (rename-file tmpfile outfile))))
> +
> (defvar comp-files-queue ()
> "List of Elisp files to be compiled.")
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 95659840ad..c1c54b3c9a 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2184,6 +2184,31 @@ If some packages are not installed propose to install them."
> (equal (cadr (assq (package-desc-name pkg) package-alist))
> pkg))
>
> +(defun package--delete-directory (dir)
> + "Delete DIR recursively.
> +In Windows move .eln and .eln.old files that can not be deleted to `package-user-dir'."
80 column lines limit. I think also this should be transparent when
native-comp-available-p say native comp is not available (for now
compiler and load machinery are bundled).
> + (cond ((eq 'windows-nt system-type)
> + (let ((retry t))
> + (while retry
> + (setf retry nil)
> + (condition-case err
> + (delete-directory dir t)
> + (file-error
> + (if (and (string= "Removing old name" (cadr err))
> + (string= "Permission denied" (caddr err))
> + (or (string-suffix-p ".eln" (cadddr err))
> + (string-suffix-p ".eln.old" (cadddr err))))
I think would be good to destructure err using something like
cl-destructuring-bind or pcase or even just using a let + some naming to
make this more readable.
> + (progn
> + (rename-file (cadddr err)
> + (make-temp-file-internal
> + (concat package-user-dir
> + (file-name-base (cadddr err)))
> + nil ".eln.old" nil)
> + t)
> + (setf retry t))
> + (signal (car err) (cdr err))))))))
> + (t (delete-directory dir t))))
> +
> (defun package-delete (pkg-desc &optional force nosave)
> "Delete package PKG-DESC.
>
> @@ -2236,7 +2261,7 @@ If NOSAVE is non-nil, the package is not removed from
> (package-desc-name pkg-used-elsewhere-by)))
> (t
> (add-hook 'post-command-hook #'package-menu--post-refresh)
> - (delete-directory dir t)
> + (package--delete-directory dir)
> ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
> ;;
> ;; NAME-readme.txt files are no longer created, but they
> diff --git a/src/alloc.c b/src/alloc.c
> index d6ba4d9790..420168ec4d 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -3119,8 +3119,7 @@ cleanup_vector (struct Lisp_Vector *vector)
> {
> struct Lisp_Native_Comp_Unit *cu =
> PSEUDOVEC_STRUCT (vector, Lisp_Native_Comp_Unit);
> - eassert (cu->handle);
> - dynlib_close (cu->handle);
> + dispose_comp_unit (cu, true);
> }
> }
>
> @@ -6117,6 +6116,8 @@ garbage_collect (void)
> if (tot_after < tot_before)
> malloc_probe (min (tot_before - tot_after, SIZE_MAX));
> }
> +
> + finish_delayed_disposal_of_comp_units ();
Could you describe why we need to call this each garbage collection?
Isn't sufficient to do it when emacs is exiting?
> }
>
> DEFUN ("garbage-collect", Fgarbage_collect, Sgarbage_collect, 0, 0, "",
> diff --git a/src/comp.c b/src/comp.c
> index dd45599cc4..77c3006c56 100644
> --- a/src/comp.c
> +++ b/src/comp.c
> @@ -413,6 +413,10 @@ load_gccjit_if_necessary (bool mandatory)
> #define CALL1I(fun, arg) \
> CALLN (Ffuncall, intern_c_string (STR (fun)), arg)
>
> +/* Like call2 but stringify and intern. */
> +#define CALL2I(fun, arg1, arg2) \
> + CALLN (Ffuncall, intern_c_string (STR (fun)), arg1, arg2)
> +
> #define DECL_BLOCK(name, func) \
> gcc_jit_block *(name) = \
> gcc_jit_function_new_block ((func), STR (name))
> @@ -3828,6 +3832,14 @@ DEFUN ("comp--release-ctxt", Fcomp__release_ctxt, Scomp__release_ctxt,
> return Qt;
> }
>
> +sigset_t oldset;
I think we have all static data at the top.
That said this is unclear to me because in comp--compile-ctxt-to-file
oldset is automatic and shadows this static, so I think we'll save in
the the automatic and later we just restore the (always zeroed) static
one.
> +static void restore_sigmask(void)
^^^
space
> +{
> + pthread_sigmask (SIG_SETMASK, &oldset, 0);
> + unblock_input ();
> +}
> +
> DEFUN ("comp--compile-ctxt-to-file", Fcomp__compile_ctxt_to_file,
> Scomp__compile_ctxt_to_file,
> 1, 1, 0,
> @@ -3849,6 +3861,8 @@ DEFUN ("comp--compile-ctxt-to-file", Fcomp__compile_ctxt_to_file,
> CALL1I (comp-data-container-idx, CALL1I (comp-ctxt-d-ephemeral, Vcomp_ctxt));
>
> sigset_t oldset;
> + ptrdiff_t count;
> +
> if (!noninteractive)
> {
> sigset_t blocked;
> @@ -3861,6 +3875,8 @@ DEFUN ("comp--compile-ctxt-to-file", Fcomp__compile_ctxt_to_file,
> sigaddset (&blocked, SIGIO);
> #endif
> pthread_sigmask (SIG_BLOCK, &blocked, &oldset);
> + count = SPECPDL_INDEX ();
> + record_unwind_protect_void(restore_sigmask);
^^^
space
> }
> emit_ctxt_code ();
>
> @@ -3899,18 +3915,10 @@ DEFUN ("comp--compile-ctxt-to-file", Fcomp__compile_ctxt_to_file,
> GCC_JIT_OUTPUT_KIND_DYNAMIC_LIBRARY,
> SSDATA (tmp_file));
>
> - /* Remove the old eln instead of copying the new one into it to get
> - a new inode and prevent crashes in case the old one is currently
> - loaded. */
> - if (!NILP (Ffile_exists_p (out_file)))
> - Fdelete_file (out_file, Qnil);
> - Frename_file (tmp_file, out_file, Qnil);
> + CALL2I(comp--replace-output-file, out_file, tmp_file);
^^^
space
>
> if (!noninteractive)
> - {
> - pthread_sigmask (SIG_SETMASK, &oldset, 0);
> - unblock_input ();
> - }
> + unbind_to(count, Qnil);
>
> return out_file;
> }
> @@ -3972,6 +3980,138 @@ helper_PSEUDOVECTOR_TYPEP_XUNTAG (Lisp_Object a, enum pvec_type code)
> }
>
>
> +/*********************************/
> +/* Disposal of compilation units */
> +/*********************************/
> +
> +#ifdef WINDOWSNT
> +#define OLD_ELN_SUFFIX_REGEXP build_string("\\.eln\\.old$")
I think instead of $ \\' is more correct.
> +static Lisp_Object all_loaded_comp_units;
All hash table in this files are postfixed as _h
> +struct delayed_comp_unit_disposal
> +{
> + struct delayed_comp_unit_disposal * next;
^^^
no space here
> + char * filename;
^^
likewise
> +};
Why an ad-hoc C structure and not a simple cons? I think it would be
simpler and safer to use just a lisp list here. Is it because we need
to add during GC? If yes, comment :)
> +struct delayed_comp_unit_disposal * delayed_comp_unit_disposal_list;
^^
likewise and the followings
> +
> +static Lisp_Object
> +returnQnil (Lisp_Object arg)
No camel case in function names.
> +{
> + return Qnil;
> +}
I think each of the following functions really needs a comment line to
explain the scope of each of them + one preamble comment to explain all
the rename mechanism how is expected to work and the two datastructures
involved.
> +static void
> +clean_comp_unit_directory (Lisp_Object filepath)
> +{
> + if (NILP (filepath))
> + return;
> + Lisp_Object files_in_dir;
> + files_in_dir = internal_condition_case_4(Fdirectory_files, filepath, Qt,
> + OLD_ELN_SUFFIX_REGEXP, Qnil, Qt, returnQnil);
80 columns
> + FOR_EACH_TAIL(files_in_dir)
> + {
> + DeleteFile (SSDATA (XCAR (files_in_dir)));
> + }
> +}
> +
> +void clean_package_user_dir_of_old_comp_units (void)
^^^
new lines
> +{
> + Lisp_Object package_user_dir = find_symbol_value (intern ("package-user-dir"));
> + if (EQ(package_user_dir, Qunbound) || !STRINGP(package_user_dir))
> + return;
> +
> + clean_comp_unit_directory(package_user_dir);
> +}
> +
> +#endif
> +
> +void dispose_comp_unit (struct Lisp_Native_Comp_Unit * comp_handle, bool delay)
^^^
likewise
> +{
> + eassert (comp_handle->handle);
> + dynlib_close (comp_handle->handle);
> +#ifdef WINDOWSNT
> + if (!delay)
> + {
> + Lisp_Object dirname = internal_condition_case_1(Ffile_name_directory,
> + build_string (comp_handle->cfile),
> + Qt,
> + returnQnil);
> + if (!NILP(dirname))
> + clean_comp_unit_directory (dirname);
I think we need to comment here why when we dispose the compilation unit
we try to clean the full directory.
> + xfree (comp_handle->cfile);
> + comp_handle->cfile = NULL;
> + }
> + else
> + {
> + struct delayed_comp_unit_disposal * head;
> + head = xmalloc (sizeof (struct delayed_comp_unit_disposal));
> + head->next = delayed_comp_unit_disposal_list;
> + head->filename = comp_handle->cfile;
> + comp_handle->cfile = NULL;
> + delayed_comp_unit_disposal_list = head;
> + }
> +#else
> + xfree (comp_handle->file);
> +#endif
> +}
Also, wasn't the plan to try to delete the file and in case of failure
to put it in a list? Here when delay is true this goes directly in the
list. Could you explain why and add comment?
> +static void
> +register_native_comp_unit (Lisp_Object comp_u)
> +{
> +#ifdef WINDOWSNT
> + static EMACS_UINT count;
> +
> + if (XFIXNUM(Fhash_table_count(all_loaded_comp_units)) >= MOST_POSITIVE_FIXNUM)
> + return;
> +
> + while (!NILP(Fgethash(make_fixnum(count), all_loaded_comp_units, Qnil)))
> + count = (count + 1) % MOST_POSITIVE_FIXNUM;
Given you are doing all of this just to get a key (we'll not use) I
think would be wise to just create the key using gensym.
> + Fputhash(make_fixnum(count), comp_u, all_loaded_comp_units);
> +#endif
> +}
>
> +void dispose_all_remaining_comp_units (void)
> +{
> +#ifdef WINDOWSNT
> + struct Lisp_Hash_Table *h = XHASH_TABLE (all_loaded_comp_units);
> +
> + for (ptrdiff_t i = 0; i < HASH_TABLE_SIZE (h); ++i)
> + {
> + Lisp_Object k = HASH_KEY (h, i);
> + if (!EQ (k, Qunbound))
> + {
> + Lisp_Object val = HASH_VALUE (h, i);
> + struct Lisp_Native_Comp_Unit * cu = XNATIVE_COMP_UNIT(val);
> + dispose_comp_unit(cu, false);
> + }
> + }
> +#endif
> +}
> +
> +void finish_delayed_disposal_of_comp_units (void)
> +{
> +#ifdef WINDOWSNT
> + for (struct delayed_comp_unit_disposal * item = delayed_comp_unit_disposal_list;
> + delayed_comp_unit_disposal_list;
> + item = delayed_comp_unit_disposal_list)
> + {
> + delayed_comp_unit_disposal_list = item->next;
> + Lisp_Object dirname
> + = internal_condition_case_1 (Ffile_name_directory,
> + build_string (item->filename), Qt,
> + returnQnil);
> + clean_comp_unit_directory (dirname);
> + xfree(item->filename);
> + xfree(item);
> + }
> +#endif
> +}
> +
> +
> /***********************************/
> /* Deferred compilation mechanism. */
> /***********************************/
> @@ -4192,6 +4332,12 @@ load_comp_unit (struct Lisp_Native_Comp_Unit *comp_u, bool loading_dump,
> d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
> for (EMACS_INT i = 0; i < d_vec_len; i++)
> data_imp_relocs[i] = AREF (comp_u->data_impure_vec, i);
> +
> + /* If we register them while dumping we will get some entries in
> + the hash table that will be duplicated when pdumper calls
> + load_comp_unit. */
> + if (!will_dump_p())
> + register_native_comp_unit (comp_u_lisp_obj);
> }
>
> if (!loading_dump)
> @@ -4349,6 +4495,9 @@ DEFUN ("native-elisp-load", Fnative_elisp_load, Snative_elisp_load, 1, 2, 0,
> if (!comp_u->handle)
> xsignal2 (Qnative_lisp_load_failed, file, build_string (dynlib_error ()));
> comp_u->file = file;
> +#ifdef WINDOWSNT
> + comp_u->cfile = xlispstrdup(file);
> +#endif
> comp_u->data_vec = Qnil;
> comp_u->lambda_gc_guard = CALLN (Fmake_hash_table, QCtest, Qeq);
> comp_u->lambda_c_name_idx_h = CALLN (Fmake_hash_table, QCtest, Qequal);
> @@ -4497,6 +4646,11 @@ syms_of_comp (void)
> staticpro (&delayed_sources);
> delayed_sources = Qnil;
>
> +#ifdef WINDOWSNT
> + staticpro (&all_loaded_comp_units);
> + all_loaded_comp_units = CALLN(Fmake_hash_table, QCweakness, Qvalue);
> +#endif
> +
> DEFVAR_LISP ("comp-ctxt", Vcomp_ctxt,
> doc: /* The compiler context. */);
> Vcomp_ctxt = Qnil;
> diff --git a/src/comp.h b/src/comp.h
> index 36e7cdf441..0b790fc7cb 100644
> --- a/src/comp.h
> +++ b/src/comp.h
> @@ -52,7 +52,15 @@ struct Lisp_Native_Comp_Unit
> /* STUFFS WE DO NOT DUMP!! */
> Lisp_Object *data_imp_relocs;
> bool loaded_once;
> +
> dynlib_handle_ptr handle;
> +#ifdef WINDOWSNT
> + /* We need to store a copy of the original file name in memory that
> + is not subject to GC because the function to dispose native
> + compilation units is called by the GC. By that time the `file'
> + string may have been sweeped. */
> + char * cfile;
> +#endif
> };
>
> #ifdef HAVE_NATIVE_COMP
> @@ -83,6 +91,14 @@ extern void syms_of_comp (void);
>
> extern void maybe_defer_native_compilation (Lisp_Object function_name,
> Lisp_Object definition);
> +
> +extern void dispose_comp_unit (struct Lisp_Native_Comp_Unit *
> comp_unit, bool delay);
> +
> +extern void finish_delayed_disposal_of_comp_units (void);
> +
> +extern void dispose_all_remaining_comp_units (void);
> +
> +extern void clean_package_user_dir_of_old_comp_units (void);
> #else
>
> static inline void
> @@ -92,6 +108,17 @@ maybe_defer_native_compilation (Lisp_Object function_name,
>
> extern void syms_of_comp (void);
>
> +static inline void dispose_comp_unit (struct Lisp_Native_Comp_Unit * comp_handle)
Newline after ret type for this and the following definitions.
> +{
> + emacs_abort();
> +}
emacs_abort is still not declared here so it does not compile. Maybe we
can just put an eassert (false).
> +static inline void dispose_all_remaining_comp_units (void)
> +{}
> +
> +static inline void clean_package_user_dir_of_old_comp_units (void)
> +{}
> +
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 23 May 2020 23:44:02 GMT)
Full text and
rfc822 format available.
Message #317 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Andrea,
Thanks for the feedback.
> Please review all the GNU code-style of this diff. I've annotated some
> to be fixed but there are quite a number more of fixes of the same kind
> to be done.
This is such a pain, I sometimes forget to change Emacs to GNU style and
sometimes I'll write GNU style at work.
> > +(defun comp--replace-output-file (outfile tmpfile)
> > + "Replace OUTFILE with TMPFILE taking the necessary steps when
> > +dealing with shared libraries that may be loaded into Emacs"
> > + (cond ((eq 'windows-nt system-type)
> > + (ignore-errors (delete-file outfile))
> > + (let ((retry t))
> > + (while retry
> > + (setf retry nil)
> > + (condition-case _
> > + (progn
> > + ;; outfile maybe recreated by another Emacs in
> > + ;; between the following two rename-file calls
> > + (if (file-exists-p outfile)
> > + (rename-file outfile (make-temp-file-internal
> > + (file-name-sans-extension
outfile)
> > + nil ".eln.old" nil)
> Isn't better to just add .old? So we will have cases of foo.eln.old.old
> instead of foo.eln.old.eln.old ?
We can't do that because foo.eln.old might exist already. This code tries to
rename an existing foo.eln to fooXXXXXX.eln.old where XXXXXX are random. We
should never try to rename fooXXXXXX.eln.old if it already exists.
> > +(defun package--delete-directory (dir)
> > + "Delete DIR recursively.
> > +In Windows move .eln and .eln.old files that can not be deleted to
`package-user-dir'."
> 80 column lines limit. I think also this should be transparent when
> native-comp-available-p say native comp is not available (for now
> compiler and load machinery are bundled).
We might be trying to delete an .eln file that another Emacs instance has
loaded. I know it sounds kind of strange, but if that is not the case then
`package--delete-directory` will just call `delete-directory`.
> I think would be good to destructure err using something like
> cl-destructuring-bind or pcase or even just using a let + some naming to
> make this more readable.
Good idea.
> > @@ -6117,6 +6116,8 @@ garbage_collect (void)
> > if (tot_after < tot_before)
> > malloc_probe (min (tot_before - tot_after, SIZE_MAX));
> > }
> > +
> > + finish_delayed_disposal_of_comp_units ();
> Could you describe why we need to call this each garbage collection?
> Isn't sufficient to do it when emacs is exiting?
I thought it was cleaner to delete *.eln.old ASAP. It would make it
necessary to
store the original files in a list, anyway.
> That said this is unclear to me because in comp--compile-ctxt-to-file
> oldset is automatic and shadows this static, so I think we'll save in
> the the automatic and later we just restore the (always zeroed) static
> one.
You are right. I'll fix it.
> +struct delayed_comp_unit_disposal
> +{
> + struct delayed_comp_unit_disposal * next;
^^^
no space here
> + char * filename;
^^
likewise
> +};
> Why an ad-hoc C structure and not a simple cons? I think it would be
> simpler and safer to use just a lisp list here. Is it because we need
> to add during GC? If yes, comment :)
Exactly. Since filename needs to be a C string, I figured that using a C
struct
would be simpler that trying to wrap the C string in a Lisp object.
> I think each of the following functions really needs a comment line to
> explain the scope of each of them + one preamble comment to explain all
> the rename mechanism how is expected to work and the two datastructures
> involved.
Sure.
> +{
> + eassert (comp_handle->handle);
> + dynlib_close (comp_handle->handle);
> +#ifdef WINDOWSNT
> + if (!delay)
> + {
> + Lisp_Object dirname =
internal_condition_case_1(Ffile_name_directory,
> + build_string
(comp_handle->cfile),
> + Qt,
> + returnQnil);
> + if (!NILP(dirname))
> + clean_comp_unit_directory (dirname);
> I think we need to comment here why when we dispose the compilation unit
> we try to clean the full directory.
This is because ideally we'd try to delete fooXXXXXX.eln.old if we are
disposing
the "foo" compilation unit and the file has been renamed. It turns out that
there is no simple way to figure out the current name of a loaded module in
Windows. GetModuleFileName returns the filename it had at the time we
loaded it.
Deleting all the *.eln.old in the directory is the (simple) workaround.
> + xfree (comp_handle->cfile);
> + comp_handle->cfile = NULL;
> + }
> + else
> + {
> + struct delayed_comp_unit_disposal * head;
> + head = xmalloc (sizeof (struct delayed_comp_unit_disposal));
> + head->next = delayed_comp_unit_disposal_list;
> + head->filename = comp_handle->cfile;
> + comp_handle->cfile = NULL;
> + delayed_comp_unit_disposal_list = head;
> + }
> +#else
> + xfree (comp_handle->file);
> +#endif
> +}
> Also, wasn't the plan to try to delete the file and in case of failure
> to put it in a list? Here when delay is true this goes directly in the
> list. Could you explain why and add comment?
There are two reasons:
We don't have a simple way of getting the current name of the module (see
above).
Deleting all the files in the directory is implemented using
Fdirectory_files,
that allocates Lisp objects, and doing that in the middle of a GC sweep is
cause
of crashes (already tried).
> Given you are doing all of this just to get a key (we'll not use) I
> think would be wise to just create the key using gensym.
Great!
Nicolas.
El sáb., 23 may. 2020 a las 19:58, Andrea Corallo (<akrl <at> sdf.org>) escribió:
> Hi Nicolas,
>
> following some comments on - Improve handling of native compilation
> etc.
>
> Please review all the GNU code-style of this diff. I've annotated some
> to be fixed but there are quite a number more of fixes of the same kind
> to be done.
>
> > When closing emacs will inspect all directories from which it loaded
> > native compilation units. If it finds a ".eln.old" file it will try to
> > delete it, if it fails that means that another Emacs instance is using
> it.
> >
> > When compiling a file we rename the file that was in the output path
> > in case it has been loaded into another Emacs instance.
> >
> > When deleting a package we move any ".eln" or ".eln.old" files in the
> > package folder that we can't delete to `package-user-dir`. Emacs will
> > check that directory when closing and delete them.
> >
> > * lisp/emacs-lisp/comp.el (comp--replace-output-file): Function called
> > from C code to finish the compilation process. It performs renaming of
> > the old file if necessary.
> > * lisp/emacs-lisp/package.el (package--delete-directory): Function to
> > delete a package directory. It moves native compilation units that it
> > can't delete to `package-user-dir'.
> > * src/alloc.c (cleanup_vector): Call dispose_comp_unit().
> > (garbage_collect): Call finish_delayed_disposal_of_comp_units().
> > * src/comp.c: Restore the signal mask using unwind-protect. Store
> > loaded native compilation units in a hash table for disposal on
> > close. Store filenames of native compilation units GC'd in a linked
> > list to finish their disposal when the GC is over.
>
> Please annotate in the changelog the new functions ex:
> finish_delayed_disposal_of_comp_units, dispose_all_remaining_comp_units,
> register_native_comp_unit are missing.
>
> > * src/comp.h: Introduce cfile member in Lisp_Native_Comp_Unit.
> > Add declarations of functions that: clean directories of unused native
> > compilation units, handle disposal of native compilation units.
> > * src/emacs.c (kill-emacs): Dispose all remaining compilation units
> > right right before calling exit().
> > * src/eval.c (internal_condition_case_3, internal_condition_case_4):
> > Add functions.
> > * src/lisp.h (internal_condition_case_3, internal_condition_case_4):
> > Add functions.
> > * src/pdumper.c (dump_do_dump_relocation): Set cfile to a copy of the
> > Lisp string specifying the file path.
>
> > diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> > index 012baf2560..1fb4cd98c0 100644
> > --- a/lisp/emacs-lisp/comp.el
> > +++ b/lisp/emacs-lisp/comp.el
> > @@ -2183,6 +2183,31 @@ comp-hint-cons
> >
> > ;; Some entry point support code.
> >
> > +(defun comp--replace-output-file (outfile tmpfile)
> > + "Replace OUTFILE with TMPFILE taking the necessary steps when
> > +dealing with shared libraries that may be loaded into Emacs"
> > + (cond ((eq 'windows-nt system-type)
> > + (ignore-errors (delete-file outfile))
> > + (let ((retry t))
> > + (while retry
> > + (setf retry nil)
> > + (condition-case _
> > + (progn
> > + ;; outfile maybe recreated by another Emacs in
> > + ;; between the following two rename-file calls
> > + (if (file-exists-p outfile)
> > + (rename-file outfile (make-temp-file-internal
> > + (file-name-sans-extension
> outfile)
> > + nil ".eln.old" nil)
>
> Isn't better to just add .old? So we will have cases of foo.eln.old.old
> instead of foo.eln.old.eln.old ?
>
> > + t))
> > + (rename-file tmpfile outfile nil))
> > + (file-already-exists (setf retry t))))))
> > + ;; Remove the old eln instead of copying the new one into it
> > + ;; to get a new inode and prevent crashes in case the old one
> > + ;; is currently loaded.
> > + (t (delete-file outfile)
> > + (rename-file tmpfile outfile))))
> > +
> > (defvar comp-files-queue ()
> > "List of Elisp files to be compiled.")
>
>
>
> > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> > index 95659840ad..c1c54b3c9a 100644
> > --- a/lisp/emacs-lisp/package.el
> > +++ b/lisp/emacs-lisp/package.el
> > @@ -2184,6 +2184,31 @@ If some packages are not installed propose to
> install them."
> > (equal (cadr (assq (package-desc-name pkg) package-alist))
> > pkg))
> >
> > +(defun package--delete-directory (dir)
> > + "Delete DIR recursively.
> > +In Windows move .eln and .eln.old files that can not be deleted to
> `package-user-dir'."
>
> 80 column lines limit. I think also this should be transparent when
> native-comp-available-p say native comp is not available (for now
> compiler and load machinery are bundled).
>
> > + (cond ((eq 'windows-nt system-type)
> > + (let ((retry t))
> > + (while retry
> > + (setf retry nil)
> > + (condition-case err
> > + (delete-directory dir t)
> > + (file-error
> > + (if (and (string= "Removing old name" (cadr err))
> > + (string= "Permission denied" (caddr err))
> > + (or (string-suffix-p ".eln" (cadddr err))
> > + (string-suffix-p ".eln.old" (cadddr err))))
>
> I think would be good to destructure err using something like
> cl-destructuring-bind or pcase or even just using a let + some naming to
> make this more readable.
>
> > + (progn
> > + (rename-file (cadddr err)
> > + (make-temp-file-internal
> > + (concat package-user-dir
> > + (file-name-base (cadddr
> err)))
> > + nil ".eln.old" nil)
> > + t)
> > + (setf retry t))
> > + (signal (car err) (cdr err))))))))
> > + (t (delete-directory dir t))))
> > +
> > (defun package-delete (pkg-desc &optional force nosave)
> > "Delete package PKG-DESC.
> >
> > @@ -2236,7 +2261,7 @@ If NOSAVE is non-nil, the package is not removed
> from
> > (package-desc-name pkg-used-elsewhere-by)))
> > (t
> > (add-hook 'post-command-hook #'package-menu--post-refresh)
> > - (delete-directory dir t)
> > + (package--delete-directory dir)
> > ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
> > ;;
> > ;; NAME-readme.txt files are no longer created, but they
> > diff --git a/src/alloc.c b/src/alloc.c
> > index d6ba4d9790..420168ec4d 100644
> > --- a/src/alloc.c
> > +++ b/src/alloc.c
> > @@ -3119,8 +3119,7 @@ cleanup_vector (struct Lisp_Vector *vector)
> > {
> > struct Lisp_Native_Comp_Unit *cu =
> > PSEUDOVEC_STRUCT (vector, Lisp_Native_Comp_Unit);
> > - eassert (cu->handle);
> > - dynlib_close (cu->handle);
> > + dispose_comp_unit (cu, true);
> > }
> > }
> >
> > @@ -6117,6 +6116,8 @@ garbage_collect (void)
> > if (tot_after < tot_before)
> > malloc_probe (min (tot_before - tot_after, SIZE_MAX));
> > }
> > +
> > + finish_delayed_disposal_of_comp_units ();
>
> Could you describe why we need to call this each garbage collection?
> Isn't sufficient to do it when emacs is exiting?
>
> > }
> >
> > DEFUN ("garbage-collect", Fgarbage_collect, Sgarbage_collect, 0, 0, "",
> > diff --git a/src/comp.c b/src/comp.c
> > index dd45599cc4..77c3006c56 100644
> > --- a/src/comp.c
> > +++ b/src/comp.c
> > @@ -413,6 +413,10 @@ load_gccjit_if_necessary (bool mandatory)
> > #define CALL1I(fun, arg) \
> > CALLN (Ffuncall, intern_c_string (STR (fun)), arg)
> >
> > +/* Like call2 but stringify and intern. */
> > +#define CALL2I(fun, arg1, arg2) \
> > + CALLN (Ffuncall, intern_c_string (STR (fun)), arg1, arg2)
> > +
> > #define DECL_BLOCK(name, func) \
> > gcc_jit_block *(name) = \
> > gcc_jit_function_new_block ((func), STR (name))
> > @@ -3828,6 +3832,14 @@ DEFUN ("comp--release-ctxt", Fcomp__release_ctxt,
> Scomp__release_ctxt,
> > return Qt;
> > }
> >
> > +sigset_t oldset;
>
> I think we have all static data at the top.
>
> That said this is unclear to me because in comp--compile-ctxt-to-file
> oldset is automatic and shadows this static, so I think we'll save in
> the the automatic and later we just restore the (always zeroed) static
> one.
>
> > +static void restore_sigmask(void)
> ^^^
> space
> > +{
> > + pthread_sigmask (SIG_SETMASK, &oldset, 0);
> > + unblock_input ();
> > +}
> > +
> > DEFUN ("comp--compile-ctxt-to-file", Fcomp__compile_ctxt_to_file,
> > Scomp__compile_ctxt_to_file,
> > 1, 1, 0,
> > @@ -3849,6 +3861,8 @@ DEFUN ("comp--compile-ctxt-to-file",
> Fcomp__compile_ctxt_to_file,
> > CALL1I (comp-data-container-idx, CALL1I (comp-ctxt-d-ephemeral,
> Vcomp_ctxt));
> >
> > sigset_t oldset;
> > + ptrdiff_t count;
> > +
> > if (!noninteractive)
> > {
> > sigset_t blocked;
> > @@ -3861,6 +3875,8 @@ DEFUN ("comp--compile-ctxt-to-file",
> Fcomp__compile_ctxt_to_file,
> > sigaddset (&blocked, SIGIO);
> > #endif
> > pthread_sigmask (SIG_BLOCK, &blocked, &oldset);
> > + count = SPECPDL_INDEX ();
> > + record_unwind_protect_void(restore_sigmask);
> ^^^
> space
>
> > }
> > emit_ctxt_code ();
> >
> > @@ -3899,18 +3915,10 @@ DEFUN ("comp--compile-ctxt-to-file",
> Fcomp__compile_ctxt_to_file,
> > GCC_JIT_OUTPUT_KIND_DYNAMIC_LIBRARY,
> > SSDATA (tmp_file));
> >
> > - /* Remove the old eln instead of copying the new one into it to get
> > - a new inode and prevent crashes in case the old one is currently
> > - loaded. */
> > - if (!NILP (Ffile_exists_p (out_file)))
> > - Fdelete_file (out_file, Qnil);
> > - Frename_file (tmp_file, out_file, Qnil);
> > + CALL2I(comp--replace-output-file, out_file, tmp_file);
> ^^^
> space
> >
> > if (!noninteractive)
> > - {
> > - pthread_sigmask (SIG_SETMASK, &oldset, 0);
> > - unblock_input ();
> > - }
> > + unbind_to(count, Qnil);
> >
> > return out_file;
> > }
> > @@ -3972,6 +3980,138 @@ helper_PSEUDOVECTOR_TYPEP_XUNTAG (Lisp_Object a,
> enum pvec_type code)
> > }
> >
> >
> > +/*********************************/
> > +/* Disposal of compilation units */
> > +/*********************************/
> > +
> > +#ifdef WINDOWSNT
> > +#define OLD_ELN_SUFFIX_REGEXP build_string("\\.eln\\.old$")
>
> I think instead of $ \\' is more correct.
>
> > +static Lisp_Object all_loaded_comp_units;
>
> All hash table in this files are postfixed as _h
>
> > +struct delayed_comp_unit_disposal
> > +{
> > + struct delayed_comp_unit_disposal * next;
> ^^^
> no space here
> > + char * filename;
> ^^
> likewise
> > +};
>
> Why an ad-hoc C structure and not a simple cons? I think it would be
> simpler and safer to use just a lisp list here. Is it because we need
> to add during GC? If yes, comment :)
>
> > +struct delayed_comp_unit_disposal * delayed_comp_unit_disposal_list;
> ^^
> likewise and the followings
> > +
> > +static Lisp_Object
> > +returnQnil (Lisp_Object arg)
>
> No camel case in function names.
>
> > +{
> > + return Qnil;
> > +}
>
> I think each of the following functions really needs a comment line to
> explain the scope of each of them + one preamble comment to explain all
> the rename mechanism how is expected to work and the two datastructures
> involved.
>
> > +static void
> > +clean_comp_unit_directory (Lisp_Object filepath)
> > +{
> > + if (NILP (filepath))
> > + return;
> > + Lisp_Object files_in_dir;
> > + files_in_dir = internal_condition_case_4(Fdirectory_files, filepath,
> Qt,
> > + OLD_ELN_SUFFIX_REGEXP, Qnil,
> Qt, returnQnil);
>
> 80 columns
>
> > + FOR_EACH_TAIL(files_in_dir)
> > + {
> > + DeleteFile (SSDATA (XCAR (files_in_dir)));
> > + }
> > +}
> > +
> > +void clean_package_user_dir_of_old_comp_units (void)
> ^^^
> new lines
> > +{
> > + Lisp_Object package_user_dir = find_symbol_value (intern
> ("package-user-dir"));
> > + if (EQ(package_user_dir, Qunbound) || !STRINGP(package_user_dir))
> > + return;
> > +
> > + clean_comp_unit_directory(package_user_dir);
> > +}
> > +
> > +#endif
> > +
> > +void dispose_comp_unit (struct Lisp_Native_Comp_Unit * comp_handle,
> bool delay)
> ^^^
> likewise
> > +{
> > + eassert (comp_handle->handle);
> > + dynlib_close (comp_handle->handle);
> > +#ifdef WINDOWSNT
> > + if (!delay)
> > + {
> > + Lisp_Object dirname =
> internal_condition_case_1(Ffile_name_directory,
> > + build_string
> (comp_handle->cfile),
> > + Qt,
> > + returnQnil);
> > + if (!NILP(dirname))
> > + clean_comp_unit_directory (dirname);
>
> I think we need to comment here why when we dispose the compilation unit
> we try to clean the full directory.
>
> > + xfree (comp_handle->cfile);
> > + comp_handle->cfile = NULL;
> > + }
> > + else
> > + {
> > + struct delayed_comp_unit_disposal * head;
> > + head = xmalloc (sizeof (struct delayed_comp_unit_disposal));
> > + head->next = delayed_comp_unit_disposal_list;
> > + head->filename = comp_handle->cfile;
> > + comp_handle->cfile = NULL;
> > + delayed_comp_unit_disposal_list = head;
> > + }
> > +#else
> > + xfree (comp_handle->file);
> > +#endif
> > +}
>
> Also, wasn't the plan to try to delete the file and in case of failure
> to put it in a list? Here when delay is true this goes directly in the
> list. Could you explain why and add comment?
>
> > +static void
> > +register_native_comp_unit (Lisp_Object comp_u)
> > +{
> > +#ifdef WINDOWSNT
> > + static EMACS_UINT count;
> > +
> > + if (XFIXNUM(Fhash_table_count(all_loaded_comp_units)) >=
> MOST_POSITIVE_FIXNUM)
> > + return;
> > +
> > + while (!NILP(Fgethash(make_fixnum(count), all_loaded_comp_units,
> Qnil)))
> > + count = (count + 1) % MOST_POSITIVE_FIXNUM;
>
> Given you are doing all of this just to get a key (we'll not use) I
> think would be wise to just create the key using gensym.
>
> > + Fputhash(make_fixnum(count), comp_u, all_loaded_comp_units);
> > +#endif
> > +}
> >
> > +void dispose_all_remaining_comp_units (void)
> > +{
> > +#ifdef WINDOWSNT
> > + struct Lisp_Hash_Table *h = XHASH_TABLE (all_loaded_comp_units);
> > +
> > + for (ptrdiff_t i = 0; i < HASH_TABLE_SIZE (h); ++i)
> > + {
> > + Lisp_Object k = HASH_KEY (h, i);
> > + if (!EQ (k, Qunbound))
> > + {
> > + Lisp_Object val = HASH_VALUE (h, i);
> > + struct Lisp_Native_Comp_Unit * cu = XNATIVE_COMP_UNIT(val);
> > + dispose_comp_unit(cu, false);
> > + }
> > + }
> > +#endif
> > +}
> > +
>
>
>
> > +void finish_delayed_disposal_of_comp_units (void)
> > +{
> > +#ifdef WINDOWSNT
> > + for (struct delayed_comp_unit_disposal * item =
> delayed_comp_unit_disposal_list;
> > + delayed_comp_unit_disposal_list;
> > + item = delayed_comp_unit_disposal_list)
> > + {
> > + delayed_comp_unit_disposal_list = item->next;
> > + Lisp_Object dirname
> > + = internal_condition_case_1 (Ffile_name_directory,
> > + build_string (item->filename), Qt,
> > + returnQnil);
> > + clean_comp_unit_directory (dirname);
> > + xfree(item->filename);
> > + xfree(item);
> > + }
> > +#endif
> > +}
> > +
> > +
> > /***********************************/
> > /* Deferred compilation mechanism. */
> > /***********************************/
> > @@ -4192,6 +4332,12 @@ load_comp_unit (struct Lisp_Native_Comp_Unit
> *comp_u, bool loading_dump,
> > d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
> > for (EMACS_INT i = 0; i < d_vec_len; i++)
> > data_imp_relocs[i] = AREF (comp_u->data_impure_vec, i);
> > +
> > + /* If we register them while dumping we will get some entries in
> > + the hash table that will be duplicated when pdumper calls
> > + load_comp_unit. */
> > + if (!will_dump_p())
> > + register_native_comp_unit (comp_u_lisp_obj);
> > }
> >
> > if (!loading_dump)
> > @@ -4349,6 +4495,9 @@ DEFUN ("native-elisp-load", Fnative_elisp_load,
> Snative_elisp_load, 1, 2, 0,
> > if (!comp_u->handle)
> > xsignal2 (Qnative_lisp_load_failed, file, build_string
> (dynlib_error ()));
> > comp_u->file = file;
> > +#ifdef WINDOWSNT
> > + comp_u->cfile = xlispstrdup(file);
> > +#endif
> > comp_u->data_vec = Qnil;
> > comp_u->lambda_gc_guard = CALLN (Fmake_hash_table, QCtest, Qeq);
> > comp_u->lambda_c_name_idx_h = CALLN (Fmake_hash_table, QCtest,
> Qequal);
> > @@ -4497,6 +4646,11 @@ syms_of_comp (void)
> > staticpro (&delayed_sources);
> > delayed_sources = Qnil;
> >
> > +#ifdef WINDOWSNT
> > + staticpro (&all_loaded_comp_units);
> > + all_loaded_comp_units = CALLN(Fmake_hash_table, QCweakness, Qvalue);
> > +#endif
> > +
> > DEFVAR_LISP ("comp-ctxt", Vcomp_ctxt,
> > doc: /* The compiler context. */);
> > Vcomp_ctxt = Qnil;
> > diff --git a/src/comp.h b/src/comp.h
> > index 36e7cdf441..0b790fc7cb 100644
> > --- a/src/comp.h
> > +++ b/src/comp.h
> > @@ -52,7 +52,15 @@ struct Lisp_Native_Comp_Unit
> > /* STUFFS WE DO NOT DUMP!! */
> > Lisp_Object *data_imp_relocs;
> > bool loaded_once;
> > +
> > dynlib_handle_ptr handle;
> > +#ifdef WINDOWSNT
> > + /* We need to store a copy of the original file name in memory that
> > + is not subject to GC because the function to dispose native
> > + compilation units is called by the GC. By that time the `file'
> > + string may have been sweeped. */
> > + char * cfile;
> > +#endif
> > };
> >
> > #ifdef HAVE_NATIVE_COMP
> > @@ -83,6 +91,14 @@ extern void syms_of_comp (void);
> >
> > extern void maybe_defer_native_compilation (Lisp_Object function_name,
> > Lisp_Object definition);
> > +
> > +extern void dispose_comp_unit (struct Lisp_Native_Comp_Unit *
> > comp_unit, bool delay);
> > +
> > +extern void finish_delayed_disposal_of_comp_units (void);
> > +
> > +extern void dispose_all_remaining_comp_units (void);
> > +
> > +extern void clean_package_user_dir_of_old_comp_units (void);
> > #else
> >
> > static inline void
> > @@ -92,6 +108,17 @@ maybe_defer_native_compilation (Lisp_Object
> function_name,
> >
> > extern void syms_of_comp (void);
> >
> > +static inline void dispose_comp_unit (struct Lisp_Native_Comp_Unit *
> comp_handle)
>
> Newline after ret type for this and the following definitions.
>
> > +{
> > + emacs_abort();
> > +}
>
> emacs_abort is still not declared here so it does not compile. Maybe we
> can just put an eassert (false).
>
> > +static inline void dispose_all_remaining_comp_units (void)
> > +{}
> > +
> > +static inline void clean_package_user_dir_of_old_comp_units (void)
> > +{}
> > +
>
>
> Thanks
>
> Andrea
>
> --
> akrl <at> sdf.org
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 24 May 2020 03:54:01 GMT)
Full text and
rfc822 format available.
Message #320 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[[[ To any NSA and FBI agents reading my email: please consider ]]]
[[[ whether defending the US Constitution against all enemies, ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]
Maybe the developers of Spacemacs should help write code to speed up
that case.
--
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 24 May 2020 08:20:02 GMT)
Full text and
rfc822 format available.
Message #323 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi Andrea,
>
> Thanks for the feedback.
>
>> Please review all the GNU code-style of this diff. I've annotated
> some
>> to be fixed but there are quite a number more of fixes of the same
> kind
>> to be done.
>
> This is such a pain, I sometimes forget to change Emacs to GNU style
> and
> sometimes I'll write GNU style at work.
Isn't this good? ;)
Joking aside, you can also set it on a directory base. Another trick if
your are not mechanically used to C GNU style is to run
check_GNU_style.sh on your patch (comes in the contrib folder of GCC).
>> > +(defun comp--replace-output-file (outfile tmpfile)
>> > + "Replace OUTFILE with TMPFILE taking the necessary steps when
>> > +dealing with shared libraries that may be loaded into Emacs"
>> > + (cond ((eq 'windows-nt system-type)
>> > + (ignore-errors (delete-file outfile))
>> > + (let ((retry t))
>> > + (while retry
>> > + (setf retry nil)
>> > + (condition-case _
>> > + (progn
>> > + ;; outfile maybe recreated by another Emacs
> in
>> > + ;; between the following two rename-file
> calls
>> > + (if (file-exists-p outfile)
>> > + (rename-file outfile
> (make-temp-file-internal
>> > +
> (file-name-sans-extension outfile)
>> > + nil ".eln.old" nil)
>
>> Isn't better to just add .old? So we will have cases of
> foo.eln.old.old
>> instead of foo.eln.old.eln.old ?
>
> We can't do that because foo.eln.old might exist already. This code
> tries to
> rename an existing foo.eln to fooXXXXXX.eln.old where XXXXXX are
> random. We
> should never try to rename fooXXXXXX.eln.old if it already exists.
Understand, I had in my mind was cleaner to add .old till the first non
used filename available was found but I guess is the same, probably even
simpler.
>> > +(defun package--delete-directory (dir)
>> > + "Delete DIR recursively.
>> > +In Windows move .eln and .eln.old files that can not be deleted
> to `package-user-dir'."
>
>> 80 column lines limit. I think also this should be transparent
> when
>> native-comp-available-p say native comp is not available (for now
>> compiler and load machinery are bundled).
>
> We might be trying to delete an .eln file that another Emacs instance
> has
> loaded. I know it sounds kind of strange, but if that is not the case
> then
> `package--delete-directory` will just call `delete-directory`.
It doesn't sound strange. I see probably is better like this for
systems running Emacs native-comp and stock at the same time.
>> I think would be good to destructure err using something like
>> cl-destructuring-bind or pcase or even just using a let + some
> naming to
>> make this more readable.
>
> Good idea.
>
>> > @@ -6117,6 +6116,8 @@ garbage_collect (void)
>> > if (tot_after < tot_before)
>> > malloc_probe (min (tot_before - tot_after, SIZE_MAX));
>> > }
>> > +
>> > + finish_delayed_disposal_of_comp_units ();
>
>> Could you describe why we need to call this each garbage
> collection?
>> Isn't sufficient to do it when emacs is exiting?
>
> I thought it was cleaner to delete *.eln.old ASAP. It would make it
> necessary to
> store the original files in a list, anyway.
The problem is that GC is called (especially by default) *very*
frequently, bounding GC performance to filesystem accesses is really not
a good idea IMO because we have no control over this last.
You could not see a difference here because:
- spaceemacs GC settings runs it way less often coming with a bigger
gc-cons-threshold by default
- GC euristincs being GC slow decides to give-up a little and accept
running less often leading to more fragmentation
- filesystem is blazingly fast
- you haven't measured ;)
>> That said this is unclear to me because in
> comp--compile-ctxt-to-file
>> oldset is automatic and shadows this static, so I think we'll save
> in
>> the the automatic and later we just restore the (always zeroed)
> static
>> one.
>
> You are right. I'll fix it.
>
>> +struct delayed_comp_unit_disposal
>> +{
>> + struct delayed_comp_unit_disposal * next;
> ^^^
> no space here
>> + char * filename;
> ^^
> likewise
>> +};
>
>> Why an ad-hoc C structure and not a simple cons? I think it would
> be
>> simpler and safer to use just a lisp list here. Is it because we
> need
>> to add during GC? If yes, comment :)
>
> Exactly. Since filename needs to be a C string, I figured that using
> a C struct
> would be simpler that trying to wrap the C string in a Lisp object.
So the reason is not to allocate lisp objs during GC correct?
>> I think each of the following functions really needs a comment line
> to
>> explain the scope of each of them + one preamble comment to explain
> all
>> the rename mechanism how is expected to work and the two
> datastructures
>> involved.
>
> Sure.
>
>> +{
>> + eassert (comp_handle->handle);
>> + dynlib_close (comp_handle->handle);
>> +#ifdef WINDOWSNT
>> + if (!delay)
>> + {
>> + Lisp_Object dirname = internal_condition_case_1
> (Ffile_name_directory,
>> + build_string
> (comp_handle->cfile),
>> + Qt,
>> + returnQnil);
>> + if (!NILP(dirname))
>> + clean_comp_unit_directory (dirname);
>
>> I think we need to comment here why when we dispose the compilation
> unit
>> we try to clean the full directory.
>
> This is because ideally we'd try to delete fooXXXXXX.eln.old if we
> are disposing
> the "foo" compilation unit and the file has been renamed. It turns
> out that
> there is no simple way to figure out the current name of a loaded
> module in
> Windows. GetModuleFileName returns the filename it had at the time we
> loaded it.
> Deleting all the *.eln.old in the directory is the (simple)
> workaround.
Uh I see. I think this is worth noting with a comment in the code too.
>> + xfree (comp_handle->cfile);
>> + comp_handle->cfile = NULL;
>> + }
>> + else
>> + {
>> + struct delayed_comp_unit_disposal * head;
>> + head = xmalloc (sizeof (struct delayed_comp_unit_disposal));
>> + head->next = delayed_comp_unit_disposal_list;
>> + head->filename = comp_handle->cfile;
>> + comp_handle->cfile = NULL;
>> + delayed_comp_unit_disposal_list = head;
>> + }
>> +#else
>> + xfree (comp_handle->file);
>> +#endif
>> +}
>
>> Also, wasn't the plan to try to delete the file and in case of
> failure
>> to put it in a list? Here when delay is true this goes directly in
> the
>> list. Could you explain why and add comment?
>
> There are two reasons:
>
> We don't have a simple way of getting the current name of the module
> (see
> above).
>
> Deleting all the files in the directory is implemented using
> Fdirectory_files,
> that allocates Lisp objects, and doing that in the middle of a GC
> sweep is cause
> of crashes (already tried).
>
>> Given you are doing all of this just to get a key (we'll not use) I
>> think would be wise to just create the key using gensym.
>
As said please annotate the ratio of this decisions in form of comments
in the code, otherwise will be trick to follow for somebody who has not
followed this thread.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 24 May 2020 17:59:02 GMT)
Full text and
rfc822 format available.
Message #326 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> The problem is that GC is called (especially by default) *very*
> frequently, bounding GC performance to filesystem accesses is really not
> a good idea IMO because we have no control over this last.
>
> You could not see a difference here because:
>
> - spaceemacs GC settings runs it way less often coming with a bigger
> gc-cons-threshold by default
>
> - GC euristincs being GC slow decides to give-up a little and accept
> running less often leading to more fragmentation
>
> - filesystem is blazingly fast
>
> - you haven't measured ;)
Actually unloading a native compilation unit is such an unfrequent operation
that all that finish_delayed_disposal_of_comp_units() does is compare a
pointer
to NULL. It will not slowdown the GC at all.
Anyway, I could change this to run on an idle timer or just handle it when
Emacs
closes. Which do you prefer?
> So the reason is not to allocate lisp objs during GC correct?
Correct.
Nicolas
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 24 May 2020 19:15:02 GMT)
Full text and
rfc822 format available.
Message #329 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> The problem is that GC is called (especially by default) *very*
>> frequently, bounding GC performance to filesystem accesses is
> really not
>> a good idea IMO because we have no control over this last.
>>
>> You could not see a difference here because:
>>
>> - spaceemacs GC settings runs it way less often coming with a
> bigger
>> gc-cons-threshold by default
>>
>> - GC euristincs being GC slow decides to give-up a little and
> accept
>> running less often leading to more fragmentation
>>
>> - filesystem is blazingly fast
>>
>> - you haven't measured ;)
>
> Actually unloading a native compilation unit is such an unfrequent
> operation
> that all that finish_delayed_disposal_of_comp_units() does is compare
> a pointer
> to NULL. It will not slowdown the GC at all.
>
> Anyway, I could change this to run on an idle timer or just handle it
> when Emacs
> closes. Which do you prefer?
What you say is correct, collecting a compilation unit is very
infrequent now. But code could decide to native compile functions each
time a performance critical operation has to be done, real world code
does that already relying on the byte-compiler.
I think to start with doing the clean-up when Emacs is closing is
sufficient, we can always add the timer in case we feel the need.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 24 May 2020 19:44:02 GMT)
Full text and
rfc822 format available.
Message #332 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I pushed a new version of the patch to my git repo.
Thanks, Nico
El dom., 24 may. 2020 a las 16:14, Andrea Corallo (<akrl <at> sdf.org>) escribió:
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> >> The problem is that GC is called (especially by default) *very*
> >> frequently, bounding GC performance to filesystem accesses is
> > really not
> >> a good idea IMO because we have no control over this last.
> >>
> >> You could not see a difference here because:
> >>
> >> - spaceemacs GC settings runs it way less often coming with a
> > bigger
> >> gc-cons-threshold by default
> >>
> >> - GC euristincs being GC slow decides to give-up a little and
> > accept
> >> running less often leading to more fragmentation
> >>
> >> - filesystem is blazingly fast
> >>
> >> - you haven't measured ;)
> >
> > Actually unloading a native compilation unit is such an unfrequent
> > operation
> > that all that finish_delayed_disposal_of_comp_units() does is compare
> > a pointer
> > to NULL. It will not slowdown the GC at all.
> >
> > Anyway, I could change this to run on an idle timer or just handle it
> > when Emacs
> > closes. Which do you prefer?
>
> What you say is correct, collecting a compilation unit is very
> infrequent now. But code could decide to native compile functions each
> time a performance critical operation has to be done, real world code
> does that already relying on the byte-compiler.
>
> I think to start with doing the clean-up when Emacs is closing is
> sufficient, we can always add the timer in case we feel the need.
>
> Thanks
>
> Andrea
>
> --
> akrl <at> sdf.org
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 12:22:02 GMT)
Full text and
rfc822 format available.
Message #335 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> I have come up with this patch to reduce the number of files
> probed when `load' is called..
Yes, also if you are working in the area feel free to remove the
effective_load_path approach because it breaks `load-prefer-newer'.
If you are working on this I won't touch it not to conflict.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 14:05:02 GMT)
Full text and
rfc822 format available.
Message #338 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> I pushed a new version of the patch to my git repo.
>
> Thanks, Nico
Hi Nico,
I see the patch is adding internal_condition_case_3 and
internal_condition_case_4 but I don't see them used. Is this a refuse
of some previous version?
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 14:29:01 GMT)
Full text and
rfc822 format available.
Message #341 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Andrea,
> I see the patch is adding internal_condition_case_3 and
> internal_condition_case_4 but I don't see them used. Is this a refuse
> of some previous version?
internal_condition_case_4 is used in clean_comp_unit_directory().
internal_condition_case_3 is not used, but I added it for completeness.
I have pushed a new patch that reduces the number of files probed when
loading.
It should work for Posix too.
Nico
El lun., 25 may. 2020 a las 11:05, Andrea Corallo (<akrl <at> sdf.org>) escribió:
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> > I pushed a new version of the patch to my git repo.
> >
> > Thanks, Nico
>
> Hi Nico,
>
> I see the patch is adding internal_condition_case_3 and
> internal_condition_case_4 but I don't see them used. Is this a refuse
> of some previous version?
>
> Thanks
>
> Andrea
>
> --
> akrl <at> sdf.org
>
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 15:07:02 GMT)
Full text and
rfc822 format available.
Message #344 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi Andrea,
>
>> I see the patch is adding internal_condition_case_3 and
>> internal_condition_case_4 but I don't see them used. Is this a
> refuse
>> of some previous version?
>
> internal_condition_case_4 is used in clean_comp_unit_directory().
> internal_condition_case_3 is not used, but I added it for
> completeness.
Ops sorry, doing last checks in parallel to other stuffs I failed to
search correctly :/
Okay, have a look just pushed.
> I have pushed a new patch that reduces the number of files probed
> when loading.
> It should work for Posix too.
Super, I'll have a look this evening.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 20:28:01 GMT)
Full text and
rfc822 format available.
Message #347 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Andrea Corallo <akrl <at> sdf.org> writes:
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
>> Hi Andrea,
>>
>>> I see the patch is adding internal_condition_case_3 and
>>> internal_condition_case_4 but I don't see them used. Is this a
>> refuse
>>> of some previous version?
>>
>> internal_condition_case_4 is used in clean_comp_unit_directory().
>> internal_condition_case_3 is not used, but I added it for
>> completeness.
>
> Ops sorry, doing last checks in parallel to other stuffs I failed to
> search correctly :/
>
> Okay, have a look just pushed.
>
>> I have pushed a new patch that reduces the number of files probed
>> when loading.
>> It should work for Posix too.
>
> Super, I'll have a look this evening.
>
> Thanks
>
> Andrea
Hi Nico,
please put the two patches in GNU code style, as mentioned you can run
check_GNU_style.sh and check against the output.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 25 May 2020 21:50:02 GMT)
Full text and
rfc822 format available.
Message #350 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> please put the two patches in GNU code style, as mentioned you can run
> check_GNU_style.sh and check against the output.
It should be correct now.
Nico
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 27 May 2020 21:03:01 GMT)
Full text and
rfc822 format available.
Message #353 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi all,
I've attached the current Nico's patch in case someone wants to engage,
here some comments from me.
> * src/fileio.c: Introduce function emacs_root_dir(). Refactor
> `expand-file-name` to use it.
> * src/lisp.h: Separate emacs_root_dir() into dos_emacs_root_dir() and
> w32_emacs_root_dir().
> * src/msdos.c: Rename emacs_root_dir() to dos_emacs_root_dir().
> * src/w32.c: Rename emacs_root_dir() to w32_emacs_root_dir().
I think would be good to mention in the commit message/changelog the
reason of this change, why it wasn't working and why it is now.
> diff --git a/src/fileio.c b/src/fileio.c
> index 2f1d2f8243..e9be811841 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -781,6 +781,18 @@ user_homedir (char const *name)
> return pw->pw_dir;
> }
>
> +static Lisp_Object
> +emacs_root_dir (void)
> +{
> +#ifdef DOS
> + return build_string (dos_emacs_root_dir ());
> +#elif defined (WINDOWSNT)
> + return build_string (w32_emacs_root_dir ());
> +#else
> + return build_string ("/");
> +#endif
> +}
I believe the indentation of these returns should be two regular spaces.
> DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
> doc: /* Convert filename NAME to absolute, and canonicalize it.
> Second arg DEFAULT-DIRECTORY is directory to start with if NAME is relative
> @@ -851,21 +863,16 @@ the root directory. */)
> }
>
> /* As a last resort, we may have to use the root as
> - default_directory below. */
> - Lisp_Object root;
> -#ifdef DOS_NT
> - /* "/" is not considered a root directory on DOS_NT, so using it
> - as default_directory causes an infinite recursion in, e.g.,
> - the following:
> + default_directory below.
>
> - (let (default-directory)
> - (expand-file-name "a"))
> + "/" is not considered a root directory on DOS_NT, so using it
> + as default_directory causes an infinite recursion in, e.g.,
> + the following:
>
> - To avoid this, we use the root of the current drive. */
> - root = build_string (emacs_root_dir ());
> -#else
> - root = build_string ("/");
> -#endif
> + (let (default-directory)
> + (expand-file-name "a"))
> +
> + To avoid this, we use the root of the current drive. */
I suspect this commentary is not very ideal here given now the code has
been moved into emacs_root_dir, maybe the commentary should go there.
> /* Use the buffer's default-directory if DEFAULT_DIRECTORY is omitted. */
> if (NILP (default_directory))
> @@ -891,13 +898,13 @@ the root directory. */)
> Lisp_Object absdir
> = STRINGP (Vinvocation_directory)
> && file_name_absolute_no_tilde_p (Vinvocation_directory)
> - ? Vinvocation_directory : root;
> + ? Vinvocation_directory : emacs_root_dir ();
> default_directory = Fexpand_file_name (dir, absdir);
> }
> }
> }
Thanks
Andrea
--
akrl <at> sdf.org
[0001-Determine-the-emacs-root-dir-only-when-necessary.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Thu, 28 May 2020 06:18:01 GMT)
Full text and
rfc822 format available.
Message #356 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Date: Wed, 27 May 2020 21:02:45 +0000
> Cc: 41242 <at> debbugs.gnu.org
>
> I've attached the current Nico's patch in case someone wants to engage,
> here some comments from me.
I agree with all your comments.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 29 May 2020 00:41:02 GMT)
Full text and
rfc822 format available.
Message #359 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi,
I have taken your comments into consideration and updated the patch.
Nicolas
[Message part 2 (text/html, inline)]
[0001-Determine-the-emacs-root-dir-only-when-necessary.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 29 May 2020 12:13:02 GMT)
Full text and
rfc822 format available.
Message #362 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi,
>
> I have taken your comments into consideration and updated the patch.
Hi thanks, looks more clear to me.
question: what if instead of using Ffile_exists we just use fopen to
check if the file exists in dump_do_dump_relocation?
I think the origin of "the trouble" is just there while checking if a
file exists, the path in discussion should be already absolute by
construction so I suspect we do not need Fexpand_file to come into play.
Haven't tried, but if it works looks to me cleaner then entering in
logic where not everything is initialized. It's true that now you have
verified that with your patch the execution path does not involve
variables to be initialized, but the logic could change in the future.
What do you think?
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 29 May 2020 13:55:02 GMT)
Full text and
rfc822 format available.
Message #365 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: 41242 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> Date: Fri, 29 May 2020 12:12:49 +0000
>
> question: what if instead of using Ffile_exists we just use fopen to
> check if the file exists in dump_do_dump_relocation?
>
> I think the origin of "the trouble" is just there while checking if a
> file exists, the path in discussion should be already absolute by
> construction so I suspect we do not need Fexpand_file to come into play.
Will that work if the files were moved?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Fri, 29 May 2020 14:27:02 GMT)
Full text and
rfc822 format available.
Message #368 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Andrea Corallo <akrl <at> sdf.org>
>> Cc: 41242 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>> Date: Fri, 29 May 2020 12:12:49 +0000
>>
>> question: what if instead of using Ffile_exists we just use fopen to
>> check if the file exists in dump_do_dump_relocation?
>>
>> I think the origin of "the trouble" is just there while checking if a
>> file exists, the path in discussion should be already absolute by
>> construction so I suspect we do not need Fexpand_file to come into play.
>
> Will that work if the files were moved?
I think so yes, the absolute path under discussion is generated on
purpose using Vinvocation_directory as follow:
pdumper.c:5304
=====
if (installation_state == UNKNOWN)
/* Check just once if is a local build or Emacs got installed. */
installation_state =
NILP (Ffile_exists_p (concat2 (Vinvocation_directory,
XCAR (comp_u->file))))
? LOCAL_BUILD : INSTALLED;
====
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 10:52:01 GMT)
Full text and
rfc822 format available.
Message #371 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Andrea Corallo <akrl <at> sdf.org> writes:
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
>> Hi,
>>
>> I have taken your comments into consideration and updated the patch.
>
> Hi thanks, looks more clear to me.
>
> question: what if instead of using Ffile_exists we just use fopen to
> check if the file exists in dump_do_dump_relocation?
>
> I think the origin of "the trouble" is just there while checking if a
> file exists, the path in discussion should be already absolute by
> construction so I suspect we do not need Fexpand_file to come into play.
>
> Haven't tried, but if it works looks to me cleaner then entering in
> logic where not everything is initialized. It's true that now you have
> verified that with your patch the execution path does not involve
> variables to be initialized, but the logic could change in the future.
>
> What do you think?
>
> Thanks
>
> Andrea
I've pushed 15c121ee0b "* Avoid calling Ffile_exists_p too early"
implementing the discussed idea.
Should do the job in Windows too, please give it a try.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 13:07:01 GMT)
Full text and
rfc822 format available.
Message #374 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> I've pushed 15c121ee0b "* Avoid calling Ffile_exists_p too early"
> implementing the discussed idea.
> Should do the job in Windows too, please give it a try.
It works well, thank you.
Last night I implemented a very similar patch, but I didn't send it
because it was too late. You beat me to it.
Nico
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 13:25:02 GMT)
Full text and
rfc822 format available.
Message #377 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I have found a few bugs in Windows.
Patches attached (they are in my repo too).
- It is still a good idea to avoid the call to getenv("emacs_dir"), if only for
performance reasons.
- We can't use "gensym" for register_native_comp_unit() because the dynamic
library may not have been loaded yet when loading a dump file.
- Commit `f5dceed09a8234548d5b3acb76d443569533cab9` "* lisp/loadup.el: Use new
'native-comp-available-p'." causes load_gccjit_if_necessary() to be called in
temacs. This didn't work because because term/w32-win.el had not been loaded
yet. In particular, we need `dynamic-library-alist` to be defined to know the
name of the libgccjit DLL. I have defined this in syms_of_emacs(). This
definition should be active only while dumping.
- This last bug is kinda confusing. I'm not sure about my diagnosis. The list
`delayed_comp_unit_disposal_list` has nodes allocated with xmalloc(). It seems
that these blocks allocated with xmalloc() get GC'd or they get corrupted
somehow and thus they don't survive until Emacs is about to close, which is
when we need the list. I solved it by allocating the data and nodes with
HeapAlloc().
Thanks, Nico.
[0001-Determine-the-emacs-root-dir-only-when-necessary.patch (application/octet-stream, attachment)]
[0002-Do-not-call-gensym-too-early-when-loading-a-dump-fil.patch (application/octet-stream, attachment)]
[0003-Fix-loading-of-libgccjit.dll-while-dumping-in-Window.patch (application/octet-stream, attachment)]
[0004-Use-Windows-allocation-APIs-for-delayed_comp_unit_di.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 14:16:01 GMT)
Full text and
rfc822 format available.
Message #380 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Nico,
my comments on "Reduce the number of files probed when finding a lisp
file.", the full patch attached in case somebody likes to engage.
> diff --git a/src/lread.c b/src/lread.c
> index 46725d9b0f..aaf4ad1a37 100644
> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -1056,33 +1056,25 @@ This uses the variables `load-suffixes' and `load-file-rep-suffixes'. */)
> {
> Lisp_Object exts = Vload_file_rep_suffixes;
> Lisp_Object suffix = XCAR (suffixes);
> - FOR_EACH_TAIL (exts)
> - lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
> + if (false
> +#ifdef HAVE_MODULES
> + || strcmp (MODULES_SUFFIX, SSDATA (suffix)) == 0
> +#ifdef MODULES_SECONDARY_SUFFIX
> + || strcmp (MODULES_SECONDARY_SUFFIX, SSDATA (suffix)) == 0
> +#endif
> +#endif
> +#ifdef HAVE_NATIVE_COMP
> + || strcmp (NATIVE_ELISP_SUFFIX, SSDATA (suffix)) == 0
> +#endif
> + )
We can also use NATIVE_COMP_FLAG instead of HAVE_NATIVE_COMP to limit
the macrology when we are not forced to ifdef out code that involves
data structures only defined with native-comp.
> + lst = Fcons (suffix, lst);
> + else
> + FOR_EACH_TAIL (exts)
> + lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
> }
> return Fnreverse (lst);
> }
>
> -static Lisp_Object
> -effective_load_path (void)
> -{
> -#ifndef HAVE_NATIVE_COMP
> - return Vload_path;
> -#else
> - Lisp_Object lp = Vload_path;
> - Lisp_Object new_lp = Qnil;
> - FOR_EACH_TAIL (lp)
> - {
> - Lisp_Object el = XCAR (lp);
> - new_lp =
> - Fcons (concat2 (Ffile_name_as_directory (el),
> - Vcomp_native_path_postfix),
> - new_lp);
> - new_lp = Fcons (el, new_lp);
> - }
> - return Fnreverse (new_lp);
> -#endif
> -}
> -
> /* Return true if STRING ends with SUFFIX. */
> bool
> suffix_p (Lisp_Object string, const char *suffix)
> @@ -1217,6 +1209,9 @@ Return t if the file exists and loads successfully. */)
> #ifdef MODULES_SECONDARY_SUFFIX
> || suffix_p (file, MODULES_SECONDARY_SUFFIX)
> #endif
> +#endif
> +#ifdef HAVE_NATIVE_COMP
> + || suffix_p (file, NATIVE_ELISP_SUFFIX)
> #endif
> )
Same as previous comment.
> must_suffix = Qnil;
> @@ -1236,8 +1231,8 @@ Return t if the file exists and loads successfully. */)
> }
>
> fd =
> - openp (effective_load_path (), file, suffixes, &found, Qnil,
> - load_prefer_newer);
> + openp (Vload_path, file, true, suffixes, &found, Qnil,
> + load_prefer_newer);
> }
>
> if (fd == -1)
> @@ -1600,7 +1595,7 @@ directories, make sure the PREDICATE function returns `dir-ok' for them. */)
> (Lisp_Object filename, Lisp_Object path, Lisp_Object suffixes, Lisp_Object predicate)
> {
> Lisp_Object file;
> - int fd = openp (path, filename, suffixes, &file, predicate, false);
> + int fd = openp (path, filename, Qnil, suffixes, &file, predicate, false);
^^^
false?
> if (NILP (predicate) && fd >= 0)
> emacs_close (fd);
> return file;
> @@ -1611,6 +1606,9 @@ directories, make sure the PREDICATE function returns `dir-ok' for them. */)
> On success, return a file descriptor (or 1 or -2 as described below).
> On failure, return -1 and set errno.
>
> + If ADD_ELN_DIR is nonzero, use the `comp-native-path-postfix'
> + variable to change the searched path if the suffix is .eln.
> +
Is there a specific reason to add a parameter to exclude/includes elns?
I'd say we want to have an homogeneous behavior across all call sites
no?
> SUFFIXES is a list of strings containing possible suffixes.
> The empty suffix is automatically added if the list is empty.
>
> @@ -1633,8 +1631,9 @@ directories, make sure the PREDICATE function returns `dir-ok' for them. */)
> or if a non-nil and non-t PREDICATE is specified. */
>
> int
> -openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> - Lisp_Object *storeptr, Lisp_Object predicate, bool newer)
> +openp (Lisp_Object path, Lisp_Object str, bool add_eln_dir,
> + Lisp_Object suffixes, Lisp_Object *storeptr, Lisp_Object predicate,
> + bool newer)
> {
> ptrdiff_t fn_size = 100;
> char buf[100];
> @@ -1643,7 +1642,7 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> ptrdiff_t want_length;
> Lisp_Object filename;
> Lisp_Object string, tail, encoded_fn, save_string;
> - ptrdiff_t max_suffix_len = 0;
> + ptrdiff_t max_extra_len = 0;
> int last_errno = ENOENT;
> int save_fd = -1;
> USE_SAFE_ALLOCA;
> @@ -1658,8 +1657,15 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> FOR_EACH_TAIL_SAFE (tail)
> {
> CHECK_STRING_CAR (tail);
> - max_suffix_len = max (max_suffix_len,
> - SBYTES (XCAR (tail)));
> + char * suf = SSDATA (XCAR (tail));
^^
no space
> + ptrdiff_t len = SBYTES (XCAR (tail));
> + if (add_eln_dir && strcmp (NATIVE_ELISP_SUFFIX, suf) == 0)
> + {
> + CHECK_STRING (Vcomp_native_path_postfix);
> + len += 2;
> + len += SBYTES (Vcomp_native_path_postfix);
> + }
> + max_extra_len = max (max_extra_len, len);
> }
>
> string = filename = encoded_fn = save_string = Qnil;
> @@ -1677,7 +1683,7 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> executable. */
> FOR_EACH_TAIL_SAFE (path)
> {
> - ptrdiff_t baselen, prefixlen;
> + ptrdiff_t dirnamelen, prefixlen, basenamewext_len;
>
> if (EQ (path, just_use_str))
> filename = str;
> @@ -1694,22 +1700,27 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> continue;
> }
>
> +
> /* Calculate maximum length of any filename made from
> this path element/specified file name and any possible suffix. */
> - want_length = max_suffix_len + SBYTES (filename);
> + want_length = max_extra_len + SBYTES (filename);
> if (fn_size <= want_length)
> {
> fn_size = 100 + want_length;
> fn = SAFE_ALLOCA (fn_size);
> }
>
> + Lisp_Object dirnamewslash = Ffile_name_directory (filename);
> + Lisp_Object basenamewext = Ffile_name_nondirectory (filename);
> +
> /* Copy FILENAME's data to FN but remove starting /: if any. */
> - prefixlen = ((SCHARS (filename) > 2
> - && SREF (filename, 0) == '/'
> - && SREF (filename, 1) == ':')
> + prefixlen = ((SCHARS (dirnamewslash) > 2
> + && SREF (dirnamewslash, 0) == '/'
> + && SREF (dirnamewslash, 1) == ':')
> ? 2 : 0);
> - baselen = SBYTES (filename) - prefixlen;
> - memcpy (fn, SDATA (filename) + prefixlen, baselen);
> + dirnamelen = SBYTES (dirnamewslash) - prefixlen;
> + basenamewext_len = SBYTES (basenamewext);
> + memcpy (fn, SDATA (dirnamewslash) + prefixlen, dirnamelen);
>
> /* Loop over suffixes. */
> AUTO_LIST1 (empty_string_only, empty_unibyte_string);
> @@ -1719,10 +1730,24 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> Lisp_Object suffix = XCAR (tail);
> ptrdiff_t fnlen, lsuffix = SBYTES (suffix);
> Lisp_Object handler;
> + bool add_native_comp_dir = add_eln_dir
> + && (strcmp (SSDATA (suffix), NATIVE_ELISP_SUFFIX) == 0);
I think this should be optimized for non native comp builds with a
NATIVE_COMP_FLAG && in front.
> + ptrdiff_t lmiddledir = add_native_comp_dir
> + ? SBYTES (Vcomp_native_path_postfix) + 1 : 0;
> +
> + if (add_native_comp_dir)
> + {
> + memcpy (fn + dirnamelen, SDATA (Vcomp_native_path_postfix),
> + lmiddledir - 1);
Vcomp_native_path_postfix is only defined with HAVE_NATIVE_COMP so I
guess this breaks the stock build and my previous suggestion is not
applicable.
> + fn[dirnamelen+(lmiddledir-1)] = '/';
Spaces around operators + -.
I could not compile this patch because non all the calls to openp has
been updated for the new parameter (my question on adding this stands).
In general please recall to check the stock build when working on
infrastructure integration, it's quite easy to break.
Generally speaking I think the behavior we want to have is that when a
.eln file is specified this is loaded without re-adding
Vcomp_native_path_postfix. I could not test it but I suspect this is
not handled.
Thanks this is good to have in when tuned.
Andrea
--
akrl <at> sdf.org
[0001-Reduce-the-number-of-files-probed-when-finding-a-lis.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 14:18:01 GMT)
Full text and
rfc822 format available.
Message #383 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> I've pushed 15c121ee0b "* Avoid calling Ffile_exists_p too early"
>> implementing the discussed idea.
>
>> Should do the job in Windows too, please give it a try.
>
> It works well, thank you.
Great
> Last night I implemented a very similar patch, but I didn't send it
> because it was too late. You beat me to it.
>
> Nico
Apologies, feel free to just drop a message to signal you are willing to
or already working on something so I don't overlap.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 14:52:01 GMT)
Full text and
rfc822 format available.
Message #386 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> I have found a few bugs in Windows.
> Patches attached (they are in my repo too).
>
> - It is still a good idea to avoid the call to getenv("emacs_dir"), if only for
> performance reasons.
I guess this should then go into master right? Probably also others
likes to review it, in case should not be discussed in this thread.
> - We can't use "gensym" for register_native_comp_unit() because the dynamic
> library may not have been loaded yet when loading a dump file.
The code LGTM, I just think instead of make_fixum you could use make_int
to have some more room (not sure how much is realistic this scenario but
comes for free).
> - Commit `f5dceed09a8234548d5b3acb76d443569533cab9` "* lisp/loadup.el: Use new
> 'native-comp-available-p'." causes load_gccjit_if_necessary() to be called in
> temacs. This didn't work because because term/w32-win.el had not been loaded
> yet. In particular, we need `dynamic-library-alist` to be defined to know the
> name of the libgccjit DLL. I have defined this in syms_of_emacs(). This
> definition should be active only while dumping.
I'll look into.
> - This last bug is kinda confusing. I'm not sure about my diagnosis. The list
> `delayed_comp_unit_disposal_list` has nodes allocated with xmalloc(). It seems
> that these blocks allocated with xmalloc() get GC'd or they get corrupted
> somehow and thus they don't survive until Emacs is about to close, which is
> when we need the list. I solved it by allocating the data and nodes with
> HeapAlloc().
I think this issue deserves to be precisely understood.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 16:27:02 GMT)
Full text and
rfc822 format available.
Message #389 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> I guess this should then go into master right? Probably also others
> likes to review it, in case should not be discussed in this thread.
Ok, later I will open a bug report about it.
> - We can't use "gensym" for register_native_comp_unit() because the dynamic
> library may not have been loaded yet when loading a dump file.
> The code LGTM, I just think instead of make_fixum you could use make_int
> to have some more room (not sure how much is realistic this scenario but
> comes for free).
See new attached patch.
> I'll look into.
Thanks.
> I think this issue deserves to be precisely understood.
I was not even close. The code crashes when iterating over the
`all_loaded_comp_units_h` weak hash table. We get a SIGSEGV when iterating over
a native compilation unit that has already been swept by the GC. For some
reason the hash table does not get updated. Maybe the code I used to iterate
over the hash table is not ok for weak hash tables?
=============
struct Lisp_Hash_Table *h = XHASH_TABLE (all_loaded_comp_units_h);
for (ptrdiff_t i = 0; i < HASH_TABLE_SIZE (h); ++i)
{
Lisp_Object k = HASH_KEY (h, i);
if (!EQ (k, Qunbound))
{
Lisp_Object val = HASH_VALUE (h, i);
struct Lisp_Native_Comp_Unit *cu = XNATIVE_COMP_UNIT (val);
dispose_comp_unit (cu, false);
}
}
=============
Thanks, Nico.
[0001-Do-not-call-gensym-too-early-when-loading-a-dump-fil.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 16:30:01 GMT)
Full text and
rfc822 format available.
Message #392 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Sat, 30 May 2020 10:23:55 -0300
> Cc: 41242 <at> debbugs.gnu.org
>
> - Commit `f5dceed09a8234548d5b3acb76d443569533cab9` "* lisp/loadup.el: Use new
> 'native-comp-available-p'." causes load_gccjit_if_necessary() to be called in
> temacs. This didn't work because because term/w32-win.el had not been loaded
> yet. In particular, we need `dynamic-library-alist` to be defined to know the
> name of the libgccjit DLL. I have defined this in syms_of_emacs(). This
> definition should be active only while dumping.
If this is only for dumping, then please make sure it has the proper
condition to be executed only at dump time.
> - This last bug is kinda confusing. I'm not sure about my diagnosis. The list
> `delayed_comp_unit_disposal_list` has nodes allocated with xmalloc(). It seems
> that these blocks allocated with xmalloc() get GC'd or they get corrupted
> somehow and thus they don't survive until Emacs is about to close, which is
> when we need the list. I solved it by allocating the data and nodes with
> HeapAlloc().
I don't understand: the malloc implementation in the Windows build
calls HeapAlloc, so I see no reason why the latter should work while
the former doesn't. There's some other factor at work here.
In any case, it's a definite no-no to call Windows specific APIs in a
general-purpose source file, so this patch as is cannot be acceptable.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 18:52:02 GMT)
Full text and
rfc822 format available.
Message #395 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> ---
> src/comp.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/comp.c b/src/comp.c
> index 32a98173d5..310ad76fbe 100644
> --- a/src/comp.c
> +++ b/src/comp.c
> @@ -4120,7 +4120,12 @@ finish_delayed_disposal_of_comp_units (void)
> register_native_comp_unit (Lisp_Object comp_u)
> {
> #ifdef WINDOWSNT
> - Fputhash (CALL1I (gensym, Qnil), comp_u, all_loaded_comp_units_h);
> + /* We have to do this since we can't use `gensym'. This function is
> + called early when loading a dump file and subr.el may not have
> + been loaded yet. */
> + static intmax_t count;
> +
> + Fputhash(make_int(count++), comp_u, all_loaded_comp_units_h);
> #endif
> }
Again as suggested, *please* run 'check_GNU_style.sh' on your patches if
you are not used to GNU code style to fix it.
Presenting a patch correctly formatted, well tested and fully understood
is a sign of respect for reviewers and the time they are going to invest
in the review process.
We are all volunteers and we all have to cope with time constraints.
Investing time in reviews means subtracting it to other activities
including working on other patches and features.
We aim for code quality rather then quantity or other metrics.
The followings are to be considered as basic features we want for all
patches (not just this) to be applied to this branch:
- Compiles and bootstrap --with-nativecomp --without-nativecomp
- Formatting is correct
Obviously we can always make mistakes that is totally okay, but does not
have to be a routine that is expected to be fixed by reviewers.
Please apply these suggestions to all patches that are submitted or
pending for review to speed-up the process so we can leave the
discussion for interesting topics.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 20:16:02 GMT)
Full text and
rfc822 format available.
Message #398 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Again as suggested, *please* run 'check_GNU_style.sh' on your patches if
> you are not used to GNU code style to fix it.
I will set it as a git hook, so I won't be able to commit unless the code is
well formatted.
> Presenting a patch correctly formatted, well tested and fully understood
> is a sign of respect for reviewers and the time they are going to invest
> in the review process.
> We are all volunteers and we all have to cope with time constraints.
> Investing time in reviews means subtracting it to other activities
> including working on other patches and features.
I have great respect for you and Eli, and for all the time you have spent
reviewing my patches. I am sorry that my lack of attention has been taken as a
lack respect for you. It will not happen again.
> We aim for code quality rather then quantity or other metrics.
> The followings are to be considered as basic features we want for all
> patches (not just this) to be applied to this branch:
> - Compiles and bootstrap --with-nativecomp --without-nativecomp
I had setup an AppVeyor instance that compiles my repo without native-comp on
Windows. I could not detect the build problems in my latest patch for some
unknown reason. I didn't expect that to happen. I will add two instances that
build the code on GNU/Linux with and without native-comp, that should help me
catch more build errors.
> - Formatting is correct
> Obviously we can always make mistakes that is totally okay, but does not
> have to be a routine that is expected to be fixed by reviewers.
> Please apply these suggestions to all patches that are submitted or
> pending for review to speed-up the process so we can leave the
> discussion for interesting topics.
I am really sorry for wasting your time like this. It will not happen again.
Nico
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 30 May 2020 20:55:02 GMT)
Full text and
rfc822 format available.
Message #401 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
I have reformatted the two bug fixes I attached earlier.
Nico
[0001-Do-not-call-gensym-too-early-when-loading-a-dump-fil.patch (application/octet-stream, attachment)]
[0002-Fix-loading-of-libgccjit.dll-while-dumping-in-Window.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 31 May 2020 08:56:02 GMT)
Full text and
rfc822 format available.
Message #404 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> I have reformatted the two bug fixes I attached earlier.
Appreciated, I've applied the two.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 31 May 2020 15:35:02 GMT)
Full text and
rfc822 format available.
Message #407 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Andrea,
> I could not compile this patch because non all the calls to openp has
> been updated for the new parameter (my question on adding this stands).
Sorry. I didn't check the GNU/Linux build.
> In general please recall to check the stock build when working on
> infrastructure integration, it's quite easy to break.
I have tested that this new patch builds and bootstraps in windows x64,
Ubuntu 18.04 amd64. Both with and without nativecomp.
> Generally speaking I think the behavior we want to have is that when a
> .eln file is specified this is loaded without re-adding
> Vcomp_native_path_postfix. I could not test it but I suspect this is
> not handled.
I tested moving company.eln to a directory in load-path without
`comp-native-path-postfix` and then ran (load "company.eln") and it was
loaded from that directory.
Nico.
[0001-Reduce-the-number-of-files-probed-when-finding-a-lis.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sun, 31 May 2020 22:42:01 GMT)
Full text and
rfc822 format available.
Message #410 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Last night I came up with a fresh idea to properly solve the problem with slow
loads. Considering that it happens when `load-path` contains hundreds of
directories I thought that a caching mechanism would help.
This "cache" would consist of a mapping of files that would get probed when
(load "foo") runs. It would be implemented as a hash table. This could get
stored in `package-user-dir` and package.el would be in charge of updating it
when installing or removing packages, just like autoloads.
The contents of this hash table could be something like this:
The directory company-20200525.101 could have a file load-cache.el with:
company -> ("company-20200525.101/eln-hash/company.eln"
"company-20200525.101/company.el"
"company-20200525.101/company.elc")
[...]
The directory helm-20200517.509 could have a file load-cache.el with:
helm -> ("helm-20200517.509/eln-hash/helm.eln"
"helm-20200517.509/helm.el"
"helm-20200517.509/helm.elc")
[...]
When `load-path` changes we could update the in-memory hash table by loading all
the load-cache.el files.
Then, when (require 'foo) runs, the loading code could look at the hash
table and only fopen() the files associated with the feature we are loading.
This would reduce the number of calls to fopen() from thousands to ~3 in the
worst case.
Of course, this feature would be disabled by default.
What do you think?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 01 Jun 2020 07:22:01 GMT)
Full text and
rfc822 format available.
Message #413 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Last night I came up with a fresh idea to properly solve the problem with slow
> loads. Considering that it happens when `load-path` contains hundreds of
> directories I thought that a caching mechanism would help.
>
> This "cache" would consist of a mapping of files that would get probed when
> (load "foo") runs. It would be implemented as a hash table. This could get
> stored in `package-user-dir` and package.el would be in charge of updating it
> when installing or removing packages, just like autoloads.
>
> The contents of this hash table could be something like this:
>
> The directory company-20200525.101 could have a file load-cache.el with:
>
> company -> ("company-20200525.101/eln-hash/company.eln"
> "company-20200525.101/company.el"
> "company-20200525.101/company.elc")
> [...]
>
> The directory helm-20200517.509 could have a file load-cache.el with:
>
> helm -> ("helm-20200517.509/eln-hash/helm.eln"
> "helm-20200517.509/helm.el"
> "helm-20200517.509/helm.elc")
> [...]
>
> When `load-path` changes we could update the in-memory hash table by loading all
> the load-cache.el files.
>
> Then, when (require 'foo) runs, the loading code could look at the hash
> table and only fopen() the files associated with the feature we are loading.
> This would reduce the number of calls to fopen() from thousands to ~3 in the
> worst case.
>
> Of course, this feature would be disabled by default.
>
> What do you think?
Hi Nico,
could you recall me why this is specific to the native-comp branch?
Isn't this a problem that should affect any Emacs on Windows?
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 01 Jun 2020 13:57:01 GMT)
Full text and
rfc822 format available.
Message #416 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Isn't this a problem that should affect any Emacs on Windows?
You are right. I will open a separate bug.
Nico
El lun., 1 jun. 2020 a las 4:21, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> > Last night I came up with a fresh idea to properly solve the problem with slow
> > loads. Considering that it happens when `load-path` contains hundreds of
> > directories I thought that a caching mechanism would help.
> >
> > This "cache" would consist of a mapping of files that would get probed when
> > (load "foo") runs. It would be implemented as a hash table. This could get
> > stored in `package-user-dir` and package.el would be in charge of updating it
> > when installing or removing packages, just like autoloads.
> >
> > The contents of this hash table could be something like this:
> >
> > The directory company-20200525.101 could have a file load-cache.el with:
> >
> > company -> ("company-20200525.101/eln-hash/company.eln"
> > "company-20200525.101/company.el"
> > "company-20200525.101/company.elc")
> > [...]
> >
> > The directory helm-20200517.509 could have a file load-cache.el with:
> >
> > helm -> ("helm-20200517.509/eln-hash/helm.eln"
> > "helm-20200517.509/helm.el"
> > "helm-20200517.509/helm.elc")
> > [...]
> >
> > When `load-path` changes we could update the in-memory hash table by loading all
> > the load-cache.el files.
> >
> > Then, when (require 'foo) runs, the loading code could look at the hash
> > table and only fopen() the files associated with the feature we are loading.
> > This would reduce the number of calls to fopen() from thousands to ~3 in the
> > worst case.
> >
> > Of course, this feature would be disabled by default.
> >
> > What do you think?
>
> Hi Nico,
>
> could you recall me why this is specific to the native-comp branch?
> Isn't this a problem that should affect any Emacs on Windows?
>
> Thanks
>
> Andrea
>
> --
> akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Mon, 01 Jun 2020 19:25:02 GMT)
Full text and
rfc822 format available.
Message #419 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi Andrea,
>
>> I could not compile this patch because non all the calls to openp has
>> been updated for the new parameter (my question on adding this stands).
>
> Sorry. I didn't check the GNU/Linux build.
>
>> In general please recall to check the stock build when working on
>> infrastructure integration, it's quite easy to break.
> I have tested that this new patch builds and bootstraps in windows x64,
> Ubuntu 18.04 amd64. Both with and without nativecomp.
>
>> Generally speaking I think the behavior we want to have is that when a
>> .eln file is specified this is loaded without re-adding
>> Vcomp_native_path_postfix. I could not test it but I suspect this is
>> not handled.
>
> I tested moving company.eln to a directory in load-path without
> `comp-native-path-postfix` and then ran (load "company.eln") and it was
> loaded from that directory.
>
> Nico.
Hi Nico,
thanks this looks better. I've two nits and two hopefully smarter
observations:
> +++ b/src/lread.c
> @@ -1056,31 +1056,25 @@ DEFUN ("get-load-suffixes", Fget_load_suffixes, Sget_load_suffixes, 0, 0, 0,
> {
> Lisp_Object exts = Vload_file_rep_suffixes;
> Lisp_Object suffix = XCAR (suffixes);
> - FOR_EACH_TAIL (exts)
> - lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
> - }
> - return Fnreverse (lst);
> -}
> + bool native_code_suffix = (NATIVE_COMP_FLAG
> + && strcmp (NATIVE_ELISP_SUFFIX, SSDATA (suffix)) == 0);
I think with the outer parentesys the second line should go be indented
at the level of NATIVE_COMP_FLAG. I'd probably remove the parenthesys
and put the newline after the equal.
> -static Lisp_Object
> -effective_load_path (void)
> -{
> -#ifndef HAVE_NATIVE_COMP
> - return Vload_path;
[...]
> +#ifdef HAVE_NATIVE_COMP
> + CHECK_STRING_CAR (tail);
> + char * suf = SSDATA (XCAR (tail));
> + if (strcmp (NATIVE_ELISP_SUFFIX, suf) == 0)
> + {
> + CHECK_STRING (Vcomp_native_path_postfix);
> + /* Here we add them in the opposite order so that nreverse
> + corrects it. */
> + extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
> + extended_suf = Fcons (Fcons (Vcomp_native_path_postfix, XCAR (tail)),
> + extended_suf);
> + }
> + else
> +#endif
> + {
> + extended_suf = Fcons (Fcons (Qnil, XCAR (tail)), extended_suf);
> + }
I think GNU style does not want these brackets if the statement is just
one.
Okay interesting stuffs:
In which folders are we going to search if we do (load "...a/path/foo.eln")?
I believe in this case we should search the file only in "...a/path/"
because the user really want to load this specific file. Am I correct?
That said IMO this logic is sufficiently complex to deserve a minimum of
testing to make sure we have it under control. Not sure if the best
place is files-tests.el or comp-tests.el.
Maybe Eli likes to gives his opinion on this last point and on the patch
in general.
Thanks
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 00:44:02 GMT)
Full text and
rfc822 format available.
Message #422 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> In which folders are we going to search if we do (load "...a/path/foo.eln")?
> I believe in this case we should search the file only in "...a/path/"
> because the user really want to load this specific file. Am I correct?
I'm not sure we want the load function to be that smart. This is from
the manual:
-----
The load function is not clever about looking at filename. In the perverse case
of a file named foo.el.el, evaluation of (load "foo.el") will indeed find it.
-----
I think we should respect that principle when dealing with .eln files, even if
it leads to trying to open absurd filenames.
I did some tests, and these are the files probed in each case:
(load "dir/foo.eln")
"{elt}/dir/foo.eln.eln"
"{elt}/dir/eln-hash/foo.eln.eln"
"{elt}/dir/foo.eln.dll"
"{elt}/dir/foo.eln.elc"
"{elt}/dir/foo.eln.elc.gz"
"{elt}/dir/foo.eln.el"
"{elt}/dir/foo.eln.el.gz"
"{elt}/dir/foo.eln"
"{elt}/dir/foo.eln.gz"
where {elt} is an element from `load-path`.
(load "C:/dir/foo.eln")
"c:/dir/foo.eln.eln"
"c:/dir/eln-hash/foo.eln.eln"
"c:/dir/foo.eln.dll"
"c:/dir/foo.eln.elc"
"c:/dir/foo.eln.elc.gz"
"c:/dir/foo.eln.el"
"c:/dir/foo.eln.el.gz"
"c:/dir/foo.eln"
"c:/dir/foo.eln.gz"
(load "dir/foo.eln" nil nil t) <- nosuffix: t
"{elt}/dir/foo.eln"
(load "C:/dir/foo.eln" nil nil t) <- nosuffix: t
"C:/dir/foo.eln"
> That said IMO this logic is sufficiently complex to deserve a minimum of
> testing to make sure we have it under control. Not sure if the best
> place is files-tests.el or comp-tests.el.
I agree about this. I am not sure what is the best way to do it. The list of
files probed is inaccessible from Lisp.
Thanks,
Nico.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 14:44:02 GMT)
Full text and
rfc822 format available.
Message #425 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> In which folders are we going to search if we do (load "...a/path/foo.eln")?
>
>> I believe in this case we should search the file only in "...a/path/"
>> because the user really want to load this specific file. Am I correct?
>
> I'm not sure we want the load function to be that smart. This is from
> the manual:
>
> -----
> The load function is not clever about looking at filename. In the perverse case
> of a file named foo.el.el, evaluation of (load "foo.el") will indeed find it.
> -----
>
> I think we should respect that principle when dealing with .eln files, even if
> it leads to trying to open absurd filenames.
Mmmh you are maybe right, but the directory and the filename are two
different things, I've to think about it. I'd be curious of other
opinions.
> I did some tests, and these are the files probed in each case:
>
> (load "dir/foo.eln")
>
> "{elt}/dir/foo.eln.eln"
> "{elt}/dir/eln-hash/foo.eln.eln"
> "{elt}/dir/foo.eln.dll"
> "{elt}/dir/foo.eln.elc"
> "{elt}/dir/foo.eln.elc.gz"
> "{elt}/dir/foo.eln.el"
> "{elt}/dir/foo.eln.el.gz"
> "{elt}/dir/foo.eln"
> "{elt}/dir/foo.eln.gz"
>
> where {elt} is an element from `load-path`.
>
> (load "C:/dir/foo.eln")
>
> "c:/dir/foo.eln.eln"
> "c:/dir/eln-hash/foo.eln.eln"
> "c:/dir/foo.eln.dll"
> "c:/dir/foo.eln.elc"
> "c:/dir/foo.eln.elc.gz"
> "c:/dir/foo.eln.el"
> "c:/dir/foo.eln.el.gz"
> "c:/dir/foo.eln"
> "c:/dir/foo.eln.gz"
>
> (load "dir/foo.eln" nil nil t) <- nosuffix: t
>
> "{elt}/dir/foo.eln"
>
> (load "C:/dir/foo.eln" nil nil t) <- nosuffix: t
>
> "C:/dir/foo.eln"
>
>> That said IMO this logic is sufficiently complex to deserve a minimum of
>> testing to make sure we have it under control. Not sure if the best
>> place is files-tests.el or comp-tests.el.
>
> I agree about this. I am not sure what is the best way to do it. The list of
> files probed is inaccessible from Lisp.
Yes but we can always compile a file and load it to see what's inside,
this is what we do in comp-tests.el, technically is not a problem if we
think is necessary.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 15:04:03 GMT)
Full text and
rfc822 format available.
Message #428 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> From: Andrea Corallo <akrl <at> sdf.org>
> Cc: 41242 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> Date: Mon, 01 Jun 2020 19:24:43 +0000
>
> In which folders are we going to search if we do (load "...a/path/foo.eln")?
>
> I believe in this case we should search the file only in "...a/path/"
> because the user really want to load this specific file. Am I correct?
Isn't that already so when we look for *.elc files?
> That said IMO this logic is sufficiently complex to deserve a minimum of
> testing to make sure we have it under control. Not sure if the best
> place is files-tests.el or comp-tests.el.
>
> Maybe Eli likes to gives his opinion on this last point and on the patch
> in general.
I think the logic should be consistent with how we search for Lisp
files in general.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 16:25:02 GMT)
Full text and
rfc822 format available.
Message #431 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Andrea Corallo <akrl <at> sdf.org>
>> Cc: 41242 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>> Date: Mon, 01 Jun 2020 19:24:43 +0000
>>
>> In which folders are we going to search if we do (load "...a/path/foo.eln")?
>>
>> I believe in this case we should search the file only in "...a/path/"
>> because the user really want to load this specific file. Am I correct?
>
> Isn't that already so when we look for *.elc files?
Yes but here the hash directory that we use to disambiguate the triplet
comes into play so we search there too. This is what Nico posted about
what we would probe for a load.
(load "C:/dir/foo.eln")
"c:/dir/foo.eln.eln"
"c:/dir/eln-hash/foo.eln.eln"
"c:/dir/foo.eln.dll"
"c:/dir/foo.eln.elc"
"c:/dir/foo.eln.elc.gz"
"c:/dir/foo.eln.el"
"c:/dir/foo.eln.el.gz"
"c:/dir/foo.eln"
"c:/dir/foo.eln.gz"
My argument was that in the case of (load "C:/dir/foo.eln") we should
try to load only "c:/dir/foo.eln" without having to look into
"c:/dir/eln-hash/".
But Nico pointed out (probably correctly) that the function is already
quite dumb regarding ignoring extentions and is probably not worth doing
an exception for this.
>> That said IMO this logic is sufficiently complex to deserve a minimum of
>> testing to make sure we have it under control. Not sure if the best
>> place is files-tests.el or comp-tests.el.
>>
>> Maybe Eli likes to gives his opinion on this last point and on the patch
>> in general.
>
> I think the logic should be consistent with how we search for Lisp
> files in general.
>
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 21:18:01 GMT)
Full text and
rfc822 format available.
Message #434 received at 41242 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi,
The recent patches broke the Windows build. There is one patch that
fixes it and another one that improves the way to check for the gccjit version
functions.
Nico.
El mar., 2 jun. 2020 a las 13:24, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Andrea Corallo <akrl <at> sdf.org>
> >> Cc: 41242 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> >> Date: Mon, 01 Jun 2020 19:24:43 +0000
> >>
> >> In which folders are we going to search if we do (load "...a/path/foo.eln")?
> >>
> >> I believe in this case we should search the file only in "...a/path/"
> >> because the user really want to load this specific file. Am I correct?
> >
> > Isn't that already so when we look for *.elc files?
>
> Yes but here the hash directory that we use to disambiguate the triplet
> comes into play so we search there too. This is what Nico posted about
> what we would probe for a load.
>
> (load "C:/dir/foo.eln")
>
> "c:/dir/foo.eln.eln"
> "c:/dir/eln-hash/foo.eln.eln"
> "c:/dir/foo.eln.dll"
> "c:/dir/foo.eln.elc"
> "c:/dir/foo.eln.elc.gz"
> "c:/dir/foo.eln.el"
> "c:/dir/foo.eln.el.gz"
> "c:/dir/foo.eln"
> "c:/dir/foo.eln.gz"
>
> My argument was that in the case of (load "C:/dir/foo.eln") we should
> try to load only "c:/dir/foo.eln" without having to look into
> "c:/dir/eln-hash/".
>
> But Nico pointed out (probably correctly) that the function is already
> quite dumb regarding ignoring extentions and is probably not worth doing
> an exception for this.
>
> >> That said IMO this logic is sufficiently complex to deserve a minimum of
> >> testing to make sure we have it under control. Not sure if the best
> >> place is files-tests.el or comp-tests.el.
> >>
> >> Maybe Eli likes to gives his opinion on this last point and on the patch
> >> in general.
> >
> > I think the logic should be consistent with how we search for Lisp
> > files in general.
> >
>
> --
> akrl <at> sdf.org
[0001-Fix-DLL-imports-of-gccjit-version-functions.patch (application/octet-stream, attachment)]
[0002-Remove-kludge-for-detecting-if-gcc_jit_version_major.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 23:09:02 GMT)
Full text and
rfc822 format available.
Message #437 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi,
>
> The recent patches broke the Windows build. There is one patch that
> fixes it and another one that improves the way to check for the gccjit version
> functions.
I've pushed "Fix DLL imports of gccjit version functions." on the second
I've some perplexity.
> Nico.
> From 8aa7673f626bfa2c12e6f9c8bbec102ce2fb72d1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Mon, 1 Jun 2020 19:56:37 -0300
> Subject: [PATCH 2/2] Remove kludge for detecting if gcc_jit_version_major can
> be called.
>
> * src/comp.c (comp-libgccjit-version): Use 'dlsym' in Posix and the
> normal function loading mechanism in Windows.
> ---
> src/comp.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/src/comp.c b/src/comp.c
> index 8e7582b3e65..084d0b6745b 100644
> --- a/src/comp.c
> +++ b/src/comp.c
> @@ -365,6 +365,8 @@ #define gcc_jit_version_major fn_gcc_jit_version_major
> #define gcc_jit_version_minor fn_gcc_jit_version_minor
> #define gcc_jit_version_patchlevel fn_gcc_jit_version_patchlevel
>
> +#else
> +#include <dlfcn.h>
> #endif
>
> static bool
> @@ -3999,15 +4001,18 @@ DEFUN ("comp-libgccjit-version", Fcomp_libgccjit_version,
> #if defined (LIBGCCJIT_HAVE_gcc_jit_version) || defined (WINDOWSNT)
> load_gccjit_if_necessary (true);
>
> - /* FIXME this kludge is quite bad. Can we dynamically load on all
> - operating systems? */
> -#pragma GCC diagnostic ignored "-Waddress"
> - return gcc_jit_version_major
> - ? list3 (make_fixnum (gcc_jit_version_major ()),
> - make_fixnum (gcc_jit_version_minor ()),
> - make_fixnum (gcc_jit_version_patchlevel ()))
> - : Qnil;
> -#pragma GCC diagnostic pop
> + bool ok_to_call;
> +
> +#if defined (WINDOWSNT)
> + ok_to_call = !!fn_gcc_jit_version_major;
> +#else
> + ok_to_call = !!dlsym (RTLD_DEFAULT, "gcc_jit_version_major");
> +#endif
> +
> + return ok_to_call ? list3 (make_fixnum (gcc_jit_version_major ()),
> + make_fixnum (gcc_jit_version_minor ()),
> + make_fixnum (gcc_jit_version_patchlevel ()))
> + : Qnil;
> #else
> return Qnil;
> #endif
First given is giving away two pragmas for adding three preprocessor
directives I'd say it is adding more kludge then what is removing.
Second this code would fail both link time and load time if the linker
cannot resolve one of these three functions making the test on the
result of dlsym in the run time pointless. To work one needs to go
through function pointers as you are doing in Window, but some macrology
is requested otherwise would be a kludge^2.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 02 Jun 2020 23:40:01 GMT)
Full text and
rfc822 format available.
Message #440 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Second this code would fail both link time and load time if the linker
> cannot resolve one of these three functions making the test on the
> result of dlsym in the run time pointless.
I thought that on GNU/Linux the dynamic linker would just resolve a function
when necessary (i.e. function call) and not when starting the executable. I
assumed that lazy binding was the default, and then the call to dlsym would
prevent a crash.
This is what the man page of 'ld' says about the '-z lazy' option:
When generating an executable or shared library, mark it to tell the dynamic
linker to defer function call resolution to the point when the function is
called (lazy binding), rather than at load time. Lazy binding is the default.
I checked and the call to gcc to link temacs does not include '-z now', so lazy
binding should be in effect and the dlsym trick should work.
Anyway, if you think that it's ugly I respect that and won't object to it not
getting merged.
Thanks, Nico.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 03 Jun 2020 13:51:02 GMT)
Full text and
rfc822 format available.
Message #443 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Second this code would fail both link time and load time if the linker
>> cannot resolve one of these three functions making the test on the
>> result of dlsym in the run time pointless.
>
> I thought that on GNU/Linux the dynamic linker would just resolve a function
> when necessary (i.e. function call) and not when starting the executable. I
> assumed that lazy binding was the default, and then the call to dlsym would
> prevent a crash.
>
> This is what the man page of 'ld' says about the '-z lazy' option:
>
> When generating an executable or shared library, mark it to tell the dynamic
> linker to defer function call resolution to the point when the function is
> called (lazy binding), rather than at load time. Lazy binding is the default.
Yes, but even if you use lazy linking this code would not link if the
linker cannot verify the presence of these functions. In this case all
users with a non 10 libgccjit.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 03 Jun 2020 15:29:01 GMT)
Full text and
rfc822 format available.
Message #446 received at 41242 <at> debbugs.gnu.org (full text, mbox):
> Yes, but even if you use lazy linking this code would not link if the
> linker cannot verify the presence of these functions. In this case all
> users with a non 10 libgccjit.
Shouldn't the first #ifdef remove the code for those users? That one checks for
LIBGCCJIT_HAVE_gcc_jit_version.
Nico.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Wed, 03 Jun 2020 16:25:01 GMT)
Full text and
rfc822 format available.
Message #449 received at 41242 <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Yes, but even if you use lazy linking this code would not link if the
>> linker cannot verify the presence of these functions. In this case all
>> users with a non 10 libgccjit.
>
> Shouldn't the first #ifdef remove the code for those users? That one checks for
> LIBGCCJIT_HAVE_gcc_jit_version.
Oh my, possibly. This code is exactly the definition of what a terrible
kludge is.
--
akrl <at> sdf.org
Reply sent
to
Andrea Corallo <akrl <at> sdf.org>:
You have taken responsibility.
(Sat, 06 Jun 2020 21:42:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Nicolas Bértolo <nicolasbertolo <at> gmail.com>:
bug acknowledged by developer.
(Sat, 06 Jun 2020 21:42:02 GMT)
Full text and
rfc822 format available.
Message #454 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
I did the suggested style modification myself and applied the patch as
e38678b268.
Thanks I'm closing this.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 06 Jun 2020 21:53:01 GMT)
Full text and
rfc822 format available.
Message #457 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
> I did the suggested style modification myself and applied the patch as
> e38678b268.
Thanks.
I didn't have much time during the week to look at how to implement the testing
part. I have just came up with an idea that is abusing file-name-handler-alist.
If for tests I add a handler that logs all operations we can indirectly access
the list of files probed.
What do you think of this as a way of testing "load"?
Nico.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 06 Jun 2020 22:33:01 GMT)
Full text and
rfc822 format available.
Message #460 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> I did the suggested style modification myself and applied the patch as
>> e38678b268.
>
> Thanks.
>
> I didn't have much time during the week to look at how to implement the testing
> part. I have just came up with an idea that is abusing file-name-handler-alist.
> If for tests I add a handler that logs all operations we can indirectly access
> the list of files probed.
>
> What do you think of this as a way of testing "load"?
>
> Nico.
Hi Nico,
mmmh sounds a bit heavy but I'm not able to tell if there is a better
and comprehensive at the same time way of testing it. Perhaps if you
are chasing the hash bug I guess is more important now.
If there's no reasonable way of testing it either we write a couple of
ad hoc tests or we say we are fine with that.
Not sure if you have seen this
https://lists.gnu.org/archive/html/emacs-devel/2020-06/msg00050.html
there is also the possibility that we are going to change radically all
the eln load mechanism.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 06 Jun 2020 22:51:01 GMT)
Full text and
rfc822 format available.
Message #463 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
> Perhaps if you are chasing the hash bug I guess is more important now.
I agree, but that bug keeps eluding me. I thought of not using a Lisp hashtable
and moving to an adhoc C list may be better.
> there is also the possibility that we are going to change radically all
> the eln load mechanism.
No I hadn't. That sounds good. But there are a few issues that I'm not sure
about. How would you build the hash? Only the filename? The full path? Both of
these options have problems. The current approach makes it easy to avoid
filename collisions and to associate .eln files with their source code.
Right now I'm trying to fix a bug that prevents the Windows build from dumping.
It only happens in 64 bits and nativecomp does not make a difference, but I
haven't been able to reproduce it in master. It occurs during the marking stage
of the GC.
I'll have to bisect the newest changes from master, as I see there has been work
on the GC fixing bugs.
Nico.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Sat, 06 Jun 2020 23:21:01 GMT)
Full text and
rfc822 format available.
Message #466 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>> Perhaps if you are chasing the hash bug I guess is more important now.
>
> I agree, but that bug keeps eluding me. I thought of not using a Lisp hashtable
> and moving to an adhoc C list may be better.
>
>> there is also the possibility that we are going to change radically all
>> the eln load mechanism.
>
> No I hadn't. That sounds good. But there are a few issues that I'm not sure
> about. How would you build the hash? Only the filename? The full path? Both of
> these options have problems. The current approach makes it easy to avoid
> filename collisions and to associate .eln files with their source code.
Yeah there are a lot of open questions. Certainly if we go for this
solution it has to be fully transparent and very robust. Please join
the thread there with your observations.
> Right now I'm trying to fix a bug that prevents the Windows build from dumping.
> It only happens in 64 bits and nativecomp does not make a difference, but I
> haven't been able to reproduce it in master. It occurs during the marking stage
> of the GC.
Mmmh strange, recent GC fixes *should* not impact you. Will be
interesting to see what's the issue.
> I'll have to bisect the newest changes from master, as I see there has been work
> on the GC fixing bugs.
>
> Nico.
Andrea
--
akrl <at> sdf.org
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 09 Jun 2020 14:16:01 GMT)
Full text and
rfc822 format available.
Message #469 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi,
I have attached a bugfix for the issue that causes crashes when closing Emacs.
Ideally I would figure out why iterating over a weak hash-table does not skip
elements that were already GC'd, but I could not do it. I feel fixing
this bug is
very important, so I used a C array to keep the list of native
compilation units.
[0001-Use-a-C-array-to-keep-the-list-of-live-native-compil.patch (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41242; Package
emacs.
(Tue, 09 Jun 2020 17:18:01 GMT)
Full text and
rfc822 format available.
Message #472 received at 41242-done <at> debbugs.gnu.org (full text, mbox):
Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
> Hi,
>
> I have attached a bugfix for the issue that causes crashes when closing Emacs.
>
> Ideally I would figure out why iterating over a weak hash-table does not skip
> elements that were already GC'd, but I could not do it. I feel fixing
> this bug is
> very important, so I used a C array to keep the list of native
> compilation units.
Hi Nico,
at a quick look your code to itereate looks correct and very similar to
other we have in the codebase so this is important to understand what is
going on. Have you tried to run a reproducer like the following?
======
;;; -*- lexical-binding: t; -*-
(setq gc-cons-threshold most-positive-fixnum)
(defun foo ())
(load (native-compile #'foo))
(setq xxx (make-hash-table :weakness 'value))
(puthash 1 (subr-native-comp-unit (symbol-function #'foo)) xxx)
(defun foo ())
(maphash (lambda (k v)
(message "%s %s" k v))
xxx)
(garbage-collect)
(maphash (lambda (k v)
(message "%s %s" k v))
xxx)
======
The thing is that 'mark_and_sweep_weak_table_contents' is called after
convetional mark and before conventional sweep. This will eventually
call 'sweep_weak_table' that sets the Qunbound as key.
Should be relativelly easy to see that the CU is sweeped there, and in
case is not see the reason.
A possibility where I bet is that something goes wrong dumping and
reviving a weak hashtable (like this is not in 'weak_hash_tables').
You can quickly check against that moving the creation of your hash such
that is created at each startup.
Hope it helps
Andrea
--
akrl <at> sdf.org
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Wed, 08 Jul 2020 11:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 5 years and 41 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.