bug#43725: 28.0.50; Include feature/native-comp into master

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Hi all,

this is to handle code-review and process whose final goal is to have
the feature/native-comp branch merged into master.

I'll try to make this as easy as possible implementing suggestions.
Also please feel free to install directly changes on the branch if you
feel.

Thanks in advance to the reviewers for the time.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Lars Ingebrigtsen <[hidden email]> writes:

> Andrea Corallo <[hidden email]> writes:
>
>> this is to handle code-review and process whose final goal is to have
>> the feature/native-comp branch merged into master.
>
> Was the patch against the trunk supposed to be included here?

Hi Lars, no it wasn't :)

In the sense that I'm not sure how do we prefer to proceed with the
review and I'm open for inputs on that.

I didn't know if every time I update the branch is worth posting here a
10K+ LOC patch, so I waited.  Also assuming the review will take some
time to go through I'm not really sure how this is practical.

OTOH as the branch is already in emacs.git should be trivial for
reviewers at any point in time to produce the diff and quote the parts
they like to discuss.

Slightly different but related: I wanted to mention that I think would
be really good if possible to retain the current history.  That is going
for a merge instead of applying a single patch.  I believe this would be
of great help for me to maintain the code in the future.

Please let me know how you prefer we proceed.

Thanks!

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
Andrea Corallo <[hidden email]> writes:

>> Was the patch against the trunk supposed to be included here?
>
> Hi Lars, no it wasn't :)
>
> In the sense that I'm not sure how do we prefer to proceed with the
> review and I'm open for inputs on that.

Right.  :-)

> I didn't know if every time I update the branch is worth posting here a
> 10K+ LOC patch, so I waited.  Also assuming the review will take some
> time to go through I'm not really sure how this is practical.
>
> OTOH as the branch is already in emacs.git should be trivial for
> reviewers at any point in time to produce the diff and quote the parts
> they like to discuss.

Sure, that's fine.  That would be

git diff origin/feature/native-comp..origin/master

?

> Slightly different but related: I wanted to mention that I think would
> be really good if possible to retain the current history.  That is going
> for a merge instead of applying a single patch.  I believe this would be
> of great help for me to maintain the code in the future.

Sure, sounds good.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Eli Zaretskii
> Cc: [hidden email]
> Date: Thu, 01 Oct 2020 07:51:16 +0000
> From: Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <[hidden email]>
>
> > git diff origin/feature/native-comp..origin/master
> >
> > ?
>
> I think something like this would add into the diff all new commits that
> where pushed to master.
>
> One option is to do the same but against the last commit from master
> included into the branch (it's easy to identify as just under the last
> merge).  ATM would be:
>
> git diff 6c0f1c26d2...origin/feature/native-comp

I suggest this:

   git diff ...origin/feature/native-comp

That's how I always produce changes introduced by a branch that
diverged from the current branch.



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Eli Zaretskii <[hidden email]> writes:

>> Cc: [hidden email]
>> Date: Thu, 01 Oct 2020 07:51:16 +0000
>> From: Andrea Corallo via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <[hidden email]>
>>
>> > git diff origin/feature/native-comp..origin/master
>> >
>> > ?
>>
>> I think something like this would add into the diff all new commits that
>> where pushed to master.
>>
>> One option is to do the same but against the last commit from master
>> included into the branch (it's easy to identify as just under the last
>> merge).  ATM would be:
>>
>> git diff 6c0f1c26d2...origin/feature/native-comp
>
> I suggest this:
>
>    git diff ...origin/feature/native-comp
>
> That's how I always produce changes introduced by a branch that
> diverged from the current branch.

Wow that's very handy thanks.

Expanding what you have posted this should work regardless what is the
current checkouted branch:

git diff $(git merge-base origin/master origin/feature/native-comp)..origin/feature/native-comp



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

> I suggest this:
>
>    git diff ...origin/feature/native-comp
>
> That's how I always produce changes introduced by a branch that
> diverged from the current branch.

Thanks; very handy.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
In reply to this post by Emacs - Bugs mailing list
Hi all,

FYI today I did some work to put the testsuite in shape also for the
native build (vanilla pass clean already).

The main challenge is related to the fact that the testsuite does large
use of primitive redefinition (tipically through `cl-letf').

As redefining primitives does not take effect in optimized code I
defined a macro (`advice-flet') with similar use but to advice instead
and put it in place in a numer of tests.  You'll see this work in
d07d7ab1a0 825e85b393.

I hope this approach is accettable (thought was good to ask for a
feedback), otherwise we can revert and find another solution.

ATM the testsuite for the native build runs still not clean, I'll finish
with cleaning it up.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Eli Zaretskii <[hidden email]> writes:

>> Date: Fri, 02 Oct 2020 19:39:29 +0000
>> From: Andrea Corallo via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <[hidden email]>
>>
>> redefining primitives does not take effect in optimized code
>
> Why doesn't it?  Can it be made to take effect, like it does in
> interpreted code?

Mmmh, I think technically we could, similarly to what we do for the
advice, synthesize compile and install a trampoline.  This would read
the symbol-function and calls what's in inside.

This trampoline installation would be triggered inside Ffset.

So yeah I think we could, if that's the preferred way I can try this
way.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
Eli Zaretskii <[hidden email]> writes:

>> Mmmh, I think technically we could, similarly to what we do for the
>> advice, synthesize compile and install a trampoline.  This would read
>> the symbol-function and calls what's in inside.
>>
>> This trampoline installation would be triggered inside Ffset.
>>
>> So yeah I think we could, if that's the preferred way I can try this
>> way.
>
> Lars, Stefan: do you agree that this is the preferred way?

I'm not really qualified to have an opinion here, but if this allows
redefining primitives, I'm all for it.  Redefining primitives is a
useful tool.

Would these trampolines be installed only if the primitives are
redefined, so there'd be no performance impact on code running normally?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Lars Ingebrigtsen <[hidden email]> writes:

> Eli Zaretskii <[hidden email]> writes:
>
>>> Mmmh, I think technically we could, similarly to what we do for the
>>> advice, synthesize compile and install a trampoline.  This would read
>>> the symbol-function and calls what's in inside.
>>>
>>> This trampoline installation would be triggered inside Ffset.
>>>
>>> So yeah I think we could, if that's the preferred way I can try this
>>> way.
>>
>> Lars, Stefan: do you agree that this is the preferred way?
>
> I'm not really qualified to have an opinion here, but if this allows
> redefining primitives, I'm all for it.  Redefining primitives is a
> useful tool.
>
> Would these trampolines be installed only if the primitives are
> redefined, so there'd be no performance impact on code running normally?

That's correct.

I did some experimentation today and also the implementation was easy as
the trampoline to be synthesized is exactly the same to what we
synthesize already for advising, essentially I just had to add the
proper trigger in fset.

At this point I'm also for going this way as it just reduce the
incompatibly surface and I don't see considerable downsides.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <[hidden email]> writes:

> Lars Ingebrigtsen <[hidden email]> writes:
>
>> Eli Zaretskii <[hidden email]> writes:
>>
>>>> Mmmh, I think technically we could, similarly to what we do for the
>>>> advice, synthesize compile and install a trampoline.  This would read
>>>> the symbol-function and calls what's in inside.
>>>>
>>>> This trampoline installation would be triggered inside Ffset.
>>>>
>>>> So yeah I think we could, if that's the preferred way I can try this
>>>> way.
>>>
>>> Lars, Stefan: do you agree that this is the preferred way?
>>
>> I'm not really qualified to have an opinion here, but if this allows
>> redefining primitives, I'm all for it.  Redefining primitives is a
>> useful tool.
>>
>> Would these trampolines be installed only if the primitives are
>> redefined, so there'd be no performance impact on code running normally?
>
> That's correct.
>
> I did some experimentation today and also the implementation was easy as
> the trampoline to be synthesized is exactly the same to what we
> synthesize already for advising, essentially I just had to add the
> proper trigger in fset.
>
> At this point I'm also for going this way as it just reduce the
> incompatibly surface and I don't see considerable downsides.
>
>   Andrea

As my understanding is that that's the consensus I've pushed the revert
of those changes in the testsuite and made Ffset effective also for
redefining primitives.

FYI with the last tweaks `make check` is clean.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Stefan Monnier
In reply to this post by Emacs - Bugs mailing list
> Lars, Stefan: do you agree that this is the preferred way?

Yes,


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
In reply to this post by Emacs - Bugs mailing list
I haven't gotten very far in reviewing the patch set (because it's very
large), and I don't have any comments so far (because it all looks very
good :-)).  Just two minor comments on building and running:

If libgccjit isn't installed, configure says this:

checking for dlfunc... no
configure: error: Installed libgccjit has failed passing the smoke test.
You can verify it yourself compiling:
<https://gcc.gnu.org/onlinedocs/jit/intro/tutorial01.html>.
Please report the issue to your distribution.
Here instructions on how to compile and install libgccjit from source:
<https://gcc.gnu.org/wiki/JIT>.

Instead of saying that it isn't installed.

Also -- starting Emacs says "Compilation started.", and it seems like
it'll say that now and then when using Emacs, too.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Lars Ingebrigtsen <[hidden email]> writes:

> I haven't gotten very far in reviewing the patch set (because it's very
> large), and I don't have any comments so far (because it all looks very
> good :-)).  Just two minor comments on building and running:
>
> If libgccjit isn't installed, configure says this:
>
> checking for dlfunc... no
> configure: error: Installed libgccjit has failed passing the smoke test.
> You can verify it yourself compiling:
> <https://gcc.gnu.org/onlinedocs/jit/intro/tutorial01.html>.
> Please report the issue to your distribution.
> Here instructions on how to compile and install libgccjit from source:
> <https://gcc.gnu.org/wiki/JIT>.
>
> Instead of saying that it isn't installed.
>
> Also -- starting Emacs says "Compilation started.", and it seems like
> it'll say that now and then when using Emacs, too.

Hi Lars,

thanks for commenting, I pushed bd27257965 and 58d85f4dbb to address
your two suggestions.

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
Andrea Corallo <[hidden email]> writes:

> thanks for commenting, I pushed bd27257965 and 58d85f4dbb to address
> your two suggestions.

Thanks; works fine now.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
In reply to this post by Emacs - Bugs mailing list
Hi all,

I was wondering: is there anything I can do to help the progress of
this?

Regards

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Lars Ingebrigtsen <[hidden email]> writes:

> Andrea Corallo <[hidden email]> writes:
>
>> I was wondering: is there anything I can do to help the progress of
>> this?
>
> There's a lot of code, and very little of it is in areas where I have
> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
> about a fifth of the diff and didn't have any comments.)
>
> I wonder whether Stefan of Eli has found time to take a look at it?
>
> There's a few compilation warnings in the native-comp tree that hasn't
> been fixed yet (mostly due to ;;;###autoload stuff)?  That should be
> fixed before merging, I think.

Hi Lars,

thanks for the reply.

Are these warnings related to compiler specific files or related to
other compilation units?

  Andrea



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Lars Ingebrigtsen
Andrea Corallo <[hidden email]> writes:

> Are these warnings related to compiler specific files or related to
> other compilation units?

I'm not quite sure I understand the question, but it's things like:

 ELC+ELN   gnus/message.elc

In end of data:
message.el:8873:1: Warning: the function ‘safe-date-to-time’ might not be
    defined at runtime.

And:

;;;###autoload
(defun safe-date-to-time (date)
  ...)

I don't see any patterns to which ;;;###autoloads are "missing" in
native-comp.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Eli Zaretskii
In reply to this post by Emacs - Bugs mailing list
> From: Lars Ingebrigtsen <[hidden email]>
> Cc: [hidden email],  Eli Zaretskii <[hidden email]>,
>   [hidden email]
> Date: Thu, 26 Nov 2020 11:10:11 +0100
>
> > I was wondering: is there anything I can do to help the progress of
> > this?
>
> There's a lot of code, and very little of it is in areas where I have
> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
> about a fifth of the diff and didn't have any comments.)
>
> I wonder whether Stefan of Eli has found time to take a look at it?

It's on my todo, so I will get to it eventually.



Reply | Threaded
Open this post in threaded view
|

bug#43725: 28.0.50; Include feature/native-comp into master

Emacs - Bugs mailing list
Eli Zaretskii <[hidden email]> writes:

>> From: Lars Ingebrigtsen <[hidden email]>
>> Cc: [hidden email],  Eli Zaretskii <[hidden email]>,
>>   [hidden email]
>> Date: Thu, 26 Nov 2020 11:10:11 +0100
>>
>> > I was wondering: is there anything I can do to help the progress of
>> > this?
>>
>> There's a lot of code, and very little of it is in areas where I have
>> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
>> about a fifth of the diff and didn't have any comments.)
>>
>> I wonder whether Stefan of Eli has found time to take a look at it?
>
> It's on my todo, so I will get to it eventually.

Please while assessing the priorities of your todo, account for the fact
that with the current user-base of the branch the flow of "important"
bugs is ATM zeroed.  If we want to have this in 28 would be wise to have
the larger coverage earlier than later to increase the verification
surface.

Thanks

  Andrea



123