Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Remove with two arguments faster for internal lists #5300

Merged
merged 1 commit into from
Jan 2, 2023

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Dec 29, 2022

Before:

gap> l := List([1..100000], i->[i]);;
gap> while Length(l)>0 do Remove(l,Length(l)); od; time;
53937

After:

gap> l := List([1..100000], i->[i]);;
gap> while Length(l)>0 do Remove(l,Length(l)); od; time;
17

Resolves #767

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: performance bugs or enhancements related to performance (improvements or regressions) release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Dec 29, 2022
@fingolfin fingolfin force-pushed the mh/faster-Remove branch 2 times, most recently from f83bd71 to ffc7647 Compare December 29, 2022 02:24
@ChrisJefferson
Copy link
Contributor

The method used to reject immutable lists and now accepts them. This mostly just means the error occurs in a different place, but it does make some changes, for example p := Immutable([1,2,3]); Remove(p,100); now works, when it used to break.

This doesn't greatly make a difference, but probably best to explicitly reject immutable lists to TryNextMethod, just to avoid changing behaviour unnessarily?

Before:

    gap> l := List([1..100000], i->[i]);;
    gap> while Length(l)>0 do Remove(l,Length(l)); od; time;
    53937

After:

    gap> l := List([1..100000], i->[i]);;
    gap> while Length(l)>0 do Remove(l,Length(l)); od; time;
    17
@fingolfin
Copy link
Member Author

Thanks for the feedback! I've changed it to explicitly test for IsMutable. No point in bogging this patch down with changes to which methods are applied -- if we want to change that, it should be in a separate PR.

Copy link
Member

@frankluebeck frankluebeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, InstallEarlyMethod looks like a good solution for this kind of performance problem.
The one argument version of Remove is already a kernel operation and again quite a bit faster than this early method.

@fingolfin fingolfin merged commit b9b64c3 into gap-system:master Jan 2, 2023
@fingolfin fingolfin deleted the mh/faster-Remove branch January 2, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce performance penalty in Remove caused by method dispatch overhead for lists?
3 participants