From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Andrea Corallo Newsgroups: gmane.emacs.bugs Subject: bug#41242: Port feature/native-comp to Windows - Improve handling of native compilation... Date: Sun, 24 May 2020 08:19:07 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="55215"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) Cc: 41242@debbugs.gnu.org To: Nicolas =?UTF-8?Q?B=C3=A9rtolo?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun May 24 10:20:12 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jclrg-000EH0-4K for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 24 May 2020 10:20:12 +0200 Original-Received: from localhost ([::1]:52776 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jclrf-0001IZ-5C for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 24 May 2020 04:20:11 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47466) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jclrW-0001GV-UJ for bug-gnu-emacs@gnu.org; Sun, 24 May 2020 04:20:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:53118) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jclrW-0008Ke-Ko for bug-gnu-emacs@gnu.org; Sun, 24 May 2020 04:20:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jclrW-0004oV-GY for bug-gnu-emacs@gnu.org; Sun, 24 May 2020 04:20:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Andrea Corallo Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 24 May 2020 08:20:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41242 X-GNU-PR-Package: emacs Original-Received: via spool by 41242-submit@debbugs.gnu.org id=B41242.159030835118427 (code B ref 41242); Sun, 24 May 2020 08:20:02 +0000 Original-Received: (at 41242) by debbugs.gnu.org; 24 May 2020 08:19:11 +0000 Original-Received: from localhost ([127.0.0.1]:36427 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jclqg-0004n8-V7 for submit@debbugs.gnu.org; Sun, 24 May 2020 04:19:11 -0400 Original-Received: from mx.sdf.org ([205.166.94.20]:58025) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jclqe-0004mz-Oo for 41242@debbugs.gnu.org; Sun, 24 May 2020 04:19:09 -0400 Original-Received: from sdf.org (ma.sdf.org [205.166.94.33]) by mx.sdf.org (8.15.2/8.14.5) with ESMTPS id 04O8J7cU021207 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits) verified NO); Sun, 24 May 2020 08:19:07 GMT Original-Received: (from akrl@localhost) by sdf.org (8.15.2/8.12.8/Submit) id 04O8J7kC006244; Sun, 24 May 2020 08:19:07 GMT In-Reply-To: ("Nicolas =?UTF-8?Q?B=C3=A9rtolo?="'s message of "Sat, 23 May 2020 20:43:06 -0300") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:180887 Archived-At: Nicolas B=C3=A9rtolo writes: > Hi Andrea, > > Thanks for the feedback. > >> Please review all the GNU code-style of this diff.=C2=A0 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) >> > + =C2=A0"Replace OUTFILE with TMPFILE taking the necessary steps when >> > +dealing with shared libraries that may be loaded into Emacs" >> > + =C2=A0(cond ((eq 'windows-nt system-type) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 (ignore-errors (delete-file outfile)) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 (let ((retry t)) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (while retry >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (setf retry nil) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (condition-case _ >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (progn >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;; ou= tfile maybe recreated by another Emacs > in >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ;; be= tween the following two rename-file > calls >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (if (= file-exists-p outfile) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 (rename-file outfile > (make-temp-file-internal >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 > (file-name-sans-extension outfile) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 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) >> > + =C2=A0"Delete DIR recursively. >> > +In Windows move .eln and .eln.old files that can not be deleted > to `package-user-dir'." > >> 80 column lines limit.=C2=A0 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) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (tot_after < tot_before) >> > =C2=A0 =C2=A0 =C2=A0 malloc_probe (min (tot_before - tot_after, SIZE_M= AX)); >> > =C2=A0 =C2=A0 =C2=A0} >> > + >> > + =C2=A0finish_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 >> +{ >> + =C2=A0struct delayed_comp_unit_disposal * next; > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^^ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0no space here >> + =C2=A0char * filename; > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ^^ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 likewise >> +}; > >> Why an ad-hoc C structure and not a simple cons?=C2=A0 I think it would > be >> simpler and safer to use just a lisp list here.=C2=A0 Is it because we > need >> to add during GC?=C2=A0 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. > >> +{ >> + =C2=A0eassert (comp_handle->handle); >> + =C2=A0dynlib_close (comp_handle->handle); >> +#ifdef WINDOWSNT >> + =C2=A0if (!delay) >> + =C2=A0 =C2=A0{ >> + =C2=A0 =C2=A0 =C2=A0Lisp_Object dirname =3D internal_condition_case_1 > (Ffile_name_directory, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0build_string > (comp_handle->cfile), >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Qt, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0returnQnil); >> + =C2=A0 =C2=A0 =C2=A0if (!NILP(dirname)) >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0clean_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. >> + =C2=A0 =C2=A0 =C2=A0xfree (comp_handle->cfile); >> + =C2=A0 =C2=A0 =C2=A0comp_handle->cfile =3D NULL; >> + =C2=A0 =C2=A0} >> + =C2=A0else >> + =C2=A0 =C2=A0{ >> + =C2=A0 =C2=A0 =C2=A0struct delayed_comp_unit_disposal * head; >> + =C2=A0 =C2=A0 =C2=A0head =3D xmalloc (sizeof (struct delayed_comp_unit= _disposal)); >> + =C2=A0 =C2=A0 =C2=A0head->next =3D delayed_comp_unit_disposal_list; >> + =C2=A0 =C2=A0 =C2=A0head->filename =3D comp_handle->cfile; >> + =C2=A0 =C2=A0 =C2=A0comp_handle->cfile =3D NULL; >> + =C2=A0 =C2=A0 =C2=A0delayed_comp_unit_disposal_list =3D head; >> + =C2=A0 =C2=A0} >> +#else >> + =C2=A0xfree (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?=C2=A0 Here when delay is true this goes directly in > the >> list.=C2=A0 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 --=20 akrl@sdf.org