bug#41242: Port feature/native-comp to Windows - Improve handling of native compilation...

[Andrea Corallo]

Nicolas Bértolo <nicolasbertolo@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@sdf.org