Add `InstallEarlyMethod` which allows installing special methods that bypass method selection (and thus its overhead) #4557

fingolfin · 2021-06-10T10:09:31Z

Resolves #955

I only converted First / FirstOp to use it as a proof of concept. Before more is done, I thought it would be good to solicit feedback. Also, since I am a bit short on time, perhaps others would like to help and contribute by converting more Foo / FooOp pairs to use this?

wilfwilson

Looks really nice to me. Here's a bit more code coverage.

tst/testinstall/kernel/opers.tst

ThomasBreuer

This is very elegant.

Later on, also ApplicableMethod and MethodsOperation should return also the early methods.
(This is no problem because MethodsOperation is currently undocumented --the records for early methods cannot have all the components that are available for ordinary methods.)

Concerning the way how to deal with the now unnecessary FirstOp, I think it can be moved to the obsolescent stuff, since it is documented only via an index entry and the explanation in the documentation of First (which should remain).

Concerning the ordinary generic methods for First, one could think about removing them, and instead dealing with these cases in the early methods.

lib/list.gi

fingolfin · 2021-06-10T15:01:20Z

I made some improvements:

better commit message
added a "getter" function EARLY_METHOD(oper,narg) so that we can improve ApplicableMethod to show early methods, and perhaps also MethodsOperation.
add InstallEarlyMethod to etc/ctags_for_gap
changed a test that referenced FirstOp to instead reference First.

@ThomasBreuer wrote:

Concerning the way how to deal with the now unnecessary FirstOp, I think it can be moved to the obsolescent stuff, since it is documented only via an index entry and the explanation in the documentation of First (which should remain).

I moved it lib/obsolete.gd, but I can't change it to DeclareObsoleteSynonym because that would break the fr package, which installs a method for FirstOp.

Concerning the ordinary generic methods for First, one could think about removing them, and instead dealing with these cases in the early methods.

I am not sure I understand. Do you mean the methods for IsListOrCollection? But then code implementing special kinds of lists or collections (as e.g. the fr package does) wouldn't be able to add specialized methods for First.

fingolfin · 2021-06-10T20:23:08Z

Now ApplicableMethod works (although there is room for improvement). One thing one could add is to store the location of the InstallEarlyMethod (like we do with InstallMethod -- this is in addition to the location where the function used as a method is declared those two are often the same, but not always). Problem is, I am not sure where to store this. Perhaps should just store this externally in some kind of lookup table, as access to this data does not need to be particularly fast.

ThomasBreuer · 2021-06-11T08:54:40Z

@fingolfin Forget about the last statement in my remark above, this was nonsense.

(These early methods are dangerous, everything that is handled by them is taken away from the method selection; they are like the "GAP kernel magic" that happens for internal objects.)

Thanks for dealing with MethodsOperation, I had not expected that this would be done already with the current pull request.
(I will have to adjust a function in the Browse package to the changed output.)
In fact, I can think of listing also the "GAP kernel magic" methods (or something equivalent to them) in the result of MethodsOperation --but not in this pull request.

ssiccha

Should we introduce a helper function EarlyMethodsOfOperation so that one can conveniently check whether early methods are installed?

(I'm gonna have a look at the rest of the PR after lunch. Edit: had to help with the kid. I'll have a look later.)

lib/oper1.g

fingolfin · 2021-06-11T13:58:32Z

@ssiccha early methods are a dangerous specialized tool. To be honest, I am not even sure yet whether it's a good idea to document it in the manual (maybe we shouldn't?). There are already two ways for accessing them: (1) via MethodsOperations, (2) via EARLY_METHOD (for low-level use only). I don't think much code will ever have to access them; rather this will be mostly useful for debugging, and MethodsOperation / ApplicableMethod cover that. As such, I don't see what we gain by adding EarlyMethodsOfOperation. Also, what would that return, exactly (the plural seems to suggest that it would iterate over all arities and adds any early method it finds to a list? but what's the usecase for that?)

@ThomasBreuer wrote:

These early methods are dangerous

Agreed, they are a sharp tool. Maybe we should mention this in their documentation. And maybe we should not (yet) hook the documentation up to the reference manual?

fingolfin · 2021-06-11T14:01:29Z

lib/oper1.g

+##  <#/GAPDoc>
+##
+BIND_GLOBAL( "InstallEarlyMethod", INSTALL_EARLY_METHOD );
+#TODO; store location info somehow


Perhaps InstallEarlyMethod should also take an (optional) info string, so that one can e.g. write something like for plain lists and a function.

But then that also has to be stored somewhere. One solution for that: change OPER(oper)->earlyMethod[n] to point to a plist which contains the function, the location of the installation, and the info string. The result will have identical overhead to what is now in this PR when there is no early method; and only a small extra overhead in the rare case where there is one.

Thoughts on this, anyone?

We could also add components earlyMethodInfoString and earlyMethodInstallLocation.

I would be happy to just call these "early method", rather than record an extra string.

they currently are called "early method" (in the sense that we use this as info string for all early methods), I just thought it might be nicer to see something like "early method for a plain list and a function"; given that no predicates are shown for an early method, so without this, the only way to have any idea what it does is to inspect its code. But yeah, it's certainly a secondary feature, just like tracking the location of the InstallEarlyMethod call (useful for debugging, but not crucial for the feature overall).

fingolfin · 2021-06-11T14:02:17Z

lib/oper1.g

+##  Unlike with regular methods, no checks are performed on the
+##  arguments. Hence any operation can have at most one such early
+##  method for each arity (i.e., one early method taking 1 argument, one
+##  early method taking 2 arguments, etc.).


Somewhere here we should point out that early methods are dangerous and if they handle something, then the user cannot override this behaviour later one with regular method installations

I've now added some text for this.

ssiccha · 2021-06-11T16:06:24Z

Re EarlyMethodsOfOperation: yes, I was thinking of having this as a debugging tool, but as you said @fingolfin, ApplicableMethod and MethodsOperation already do everything we need.

I think that if there is an early method installed for an operation, then ApplicableMethod and MethodsOperation should make that as obvious as possible. MethodsOperation does that already IMO. ApplicableMethod should already at printlevel 1 make clear whether a method is early.

ssiccha · 2021-06-11T16:18:28Z

I'm in favor of documenting this in the reference manual. If someone new to GAP decides to have a look at functions like First, then not having InstallEarlyMethod documented makes this harder to understand. It should definitely come with a big fat warning though.

We could not document this in v4.12.0, test it out a bit and add "Document InstallEarlyMethod" as an issue to a v4.12.1 milestone. If we don't document it now, I imagine we'll forget about it and never put it into the reference manual.

ssiccha

I like this. 👍

ChrisJefferson · 2021-06-11T16:28:54Z

Just as a sanity check, I did a bit of benchmarking on this, but (as I expected) it's basically impossible to get the time taken to show up.

ChrisJefferson · 2021-06-11T16:39:18Z

lib/oper1.g

+##  operation <A>opr</A>. An early method is special in that it bypasses
+##  method dispatch, and is always the first method to be called when
+##  invoking the operation. This can be used to avoid method selection
+##  overhead for certain special cases.


I wondered if we should add something to clarify that this operation is called even if the arguments to not satisfy the argument filters for the method (I realise technically there was never an explicit check for these, they are enforced in InstallMethod, but I thought it might be worth clarifying. This could be done as part of your "is dangerous" comment a bit later, if that was added.

I've now added some text for this.

fingolfin · 2021-06-14T11:56:28Z

@ssiccha wrote:

ApplicableMethod should already at printlevel 1 make clear whether a method is early.

But it does? Or what are what would you like to see added here:

gap> ApplicableMethod(First, [ [1,2,3] ], 1);
#I  Searching Method for First with 1 arguments:
#I  Total: 2 entries
#I  Method 1: ``early method'' at /Users/mhorn/Projekte/GAP/gap.spielwiese/lib/list.gi:2815 , value: infinity
function( C ) ... end

Of course one could do it nicer and e.g. remove the value: infinity etc., but to me that's fine tuning, and someone else who wants it can submit a PR for it after this has been merged.

This installs a special "early" function method for a given operation. An early method is special in that it bypasses method dispatch, and is always the first method to be called when invoking the operation. This can be used to avoid method selection overhead for certain special cases. Unlike with regular methods, no checks are performed on the arguments. Hence any operation can have at most one such early method for each arity (i.e., one early method taking 1 argument, one early method taking 2 arguments, etc.). If an early method detects that it is not applicable, it can resume regular method dispatch by invoking `TryNextMethod`. For example, we install early methods for `First` that deal with internal lists, for which computing their types (and hence method selection) can be very expensive.

ThomasBreuer · 2021-06-14T14:26:47Z

I think this pull request can be merged.

ssiccha · 2021-06-16T08:10:35Z

@ssiccha wrote:

ApplicableMethod should already at printlevel 1 make clear whether a method is early.

But it does? Or what are what would you like to see added here:
gap> ApplicableMethod(First, [ [1,2,3] ], 1);
#I  Searching Method for First with 1 arguments:
#I  Total: 2 entries
#I  Method 1: ``early method'' at /Users/mhorn/Projekte/GAP/gap.spielwiese/lib/list.gi:2815 , value: infinity
function( C ) ... end
Of course one could do it nicer and e.g. remove the value: infinity etc., but to me that's fine tuning, and someone else who wants it can submit a PR for it after this has been merged.

I must have missed your code for ApplicableMethod.

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jun 10, 2021

fingolfin requested review from ChrisJefferson, ssiccha and ThomasBreuer June 10, 2021 10:09

This was referenced Jun 10, 2021

Add basic operations for row/column reductions in matrix objects #4517

Merged

Method dispatch idea: InstallEarlyMethod #955

Closed

wilfwilson reviewed Jun 10, 2021

View reviewed changes

tst/testinstall/kernel/opers.tst Show resolved Hide resolved

ThomasBreuer approved these changes Jun 10, 2021

View reviewed changes

lib/list.gi Outdated Show resolved Hide resolved

fingolfin force-pushed the mh/InstallEarlyMethod branch 2 times, most recently from e083583 to ca7d68b Compare June 10, 2021 14:59

fingolfin force-pushed the mh/InstallEarlyMethod branch 3 times, most recently from ea495ca to 140c61e Compare June 10, 2021 20:21

ssiccha reviewed Jun 11, 2021

View reviewed changes

lib/oper1.g Show resolved Hide resolved

fingolfin commented Jun 11, 2021

View reviewed changes

ssiccha approved these changes Jun 11, 2021

View reviewed changes

ChrisJefferson approved these changes Jun 11, 2021

View reviewed changes

ChrisJefferson reviewed Jun 11, 2021

View reviewed changes

fingolfin force-pushed the mh/InstallEarlyMethod branch from 140c61e to 3dc5965 Compare June 14, 2021 12:04

fingolfin merged commit 2aa151a into gap-system:master Jun 14, 2021

fingolfin deleted the mh/InstallEarlyMethod branch June 14, 2021 15:18

fingolfin mentioned this pull request Jun 15, 2021

Improve and use operations for row/column reductions in matrix objects #4565

Open

4 tasks

fingolfin mentioned this pull request Dec 7, 2021

Use InstallEarlyMethod more #4714

Open

fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes kind: new feature and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `InstallEarlyMethod` which allows installing special methods that bypass method selection (and thus its overhead) #4557

Add `InstallEarlyMethod` which allows installing special methods that bypass method selection (and thus its overhead) #4557

fingolfin commented Jun 10, 2021

wilfwilson left a comment

ThomasBreuer left a comment

fingolfin commented Jun 10, 2021

fingolfin commented Jun 10, 2021

ThomasBreuer commented Jun 11, 2021

ssiccha left a comment •

edited

Loading

fingolfin commented Jun 11, 2021

fingolfin Jun 11, 2021

ssiccha Jun 11, 2021

ChrisJefferson Jun 11, 2021

fingolfin Jun 14, 2021 •

edited

Loading

fingolfin Jun 11, 2021

fingolfin Jun 14, 2021

ssiccha commented Jun 11, 2021 •

edited

Loading

ssiccha commented Jun 11, 2021

ssiccha left a comment

ChrisJefferson commented Jun 11, 2021

ChrisJefferson Jun 11, 2021 •

edited

Loading

fingolfin Jun 14, 2021

fingolfin commented Jun 14, 2021

ThomasBreuer commented Jun 14, 2021

ssiccha commented Jun 16, 2021

Add InstallEarlyMethod which allows installing special methods that bypass method selection (and thus its overhead) #4557

Add InstallEarlyMethod which allows installing special methods that bypass method selection (and thus its overhead) #4557

Conversation

fingolfin commented Jun 10, 2021

wilfwilson left a comment

Choose a reason for hiding this comment

ThomasBreuer left a comment

Choose a reason for hiding this comment

fingolfin commented Jun 10, 2021

fingolfin commented Jun 10, 2021

ThomasBreuer commented Jun 11, 2021

ssiccha left a comment • edited Loading

Choose a reason for hiding this comment

fingolfin commented Jun 11, 2021

fingolfin Jun 11, 2021

Choose a reason for hiding this comment

ssiccha Jun 11, 2021

Choose a reason for hiding this comment

ChrisJefferson Jun 11, 2021

Choose a reason for hiding this comment

fingolfin Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

fingolfin Jun 11, 2021

Choose a reason for hiding this comment

fingolfin Jun 14, 2021

Choose a reason for hiding this comment

ssiccha commented Jun 11, 2021 • edited Loading

ssiccha commented Jun 11, 2021

ssiccha left a comment

Choose a reason for hiding this comment

ChrisJefferson commented Jun 11, 2021

ChrisJefferson Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

fingolfin Jun 14, 2021

Choose a reason for hiding this comment

fingolfin commented Jun 14, 2021

ThomasBreuer commented Jun 14, 2021

ssiccha commented Jun 16, 2021

Add `InstallEarlyMethod` which allows installing special methods that bypass method selection (and thus its overhead) #4557

Add `InstallEarlyMethod` which allows installing special methods that bypass method selection (and thus its overhead) #4557

ssiccha left a comment •

edited

Loading

fingolfin Jun 14, 2021 •

edited

Loading

ssiccha commented Jun 11, 2021 •

edited

Loading

ChrisJefferson Jun 11, 2021 •

edited

Loading