8000 Remove a lot of uses of IMPL-UNWRAP-LIST by MasterDuke17 · Pull Request #5887 · rakudo/rakudo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove a lot of uses of IMPL-UNWRAP-LIST #5887

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove a lot of uses of IMPL-UNWRAP-LIST #5887

wants to merge 2 commits into from
+198 −205

Conversation

MasterDuke17
Copy link
Contributor

Removing these ones doesn't break any spectests.

There are more than can be removed, but not all, and I haven't yet figured out exactly which.

niner
niner requested changes May 25, 2025
View reviewed changes
Copy link
Collaborator
@niner niner left a comment

Choose a reason for hiding this comment

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

The commit message is misleading. The patch removes the WRAP, not the UNWRAP calls.

MasterDuke17 added a commit to MasterDuke17/rakudo that referenced this pull request May 25, 2025
@MasterDuke17
WRAP the results of get-implicit-lookups() before indexing
89b40fb 
"[It] is a public API and as such can be overridden by a Raku class
which would return a List, not a VMArray. Thus we really should
IMPL-UNWRAP-LIST the result before indexing into it."
 - rakudo#5887 (comment)
@MasterDuke17 MasterDuke17 force-pushed the remove_a_lot_of_IMPL-UNWRAP-LISTs branch from 2501465 to 89b40fb Compare May 25, 2025 12:48
niner
niner reviewed May 25, 2025
View reviewed changes
MasterDuke17 added a commit to MasterDuke17/rakudo that referenced this pull request May 26, 2025
@MasterDuke17
UNWRAP the results of get-implicit-lookups() before indexing
589178a 
"[It] is a public API and as such can be overridden by a Raku class
which would return a List, not a VMArray. Thus we really should
IMPL-UNWRAP-LIST the result before indexing into it."
 - rakudo#5887 (comment)
@MasterDuke17 MasterDuke17 force-pushed the remove_a_lot_of_IMPL-UNWRAP-LISTs branch from 89b40fb to 589178a Compare May 26, 2025 14:50
@niner
Copy link
Collaborator
niner commented May 26, 2025

I wonder what the performance impact of this change is? For sure it's positive, but how much?

@MasterDuke17
Copy link
Contributor Author

I wonder what the performance impact of this change is? For sure it's positive, but how much?

I did RAKUDO_RAKUAST=1 raku --profile-compile=core.e.comp.main-RAKUAST.sql rakudo/gen/moar/CORE.e.setting on main and this branch, the profiles show 11,137 calls to IMPL-WRAP-LIST on main and 7,150 calls on the branch. And hyperfine reported the time to run RAKUDO_RAKUAST=1 raku rakudo/gen/moar/CORE.e.setting dropped from ~1.96s to ~1.91s.

@MasterDuke17
Copy link
Contributor Author

And hyperfine reported the time to run RAKUDO_RAKUAST=1 raku rakudo/gen/moar/CORE.e.setting dropped from ~1.96s to ~1.91s.

FWIW, without RAKUDO_RAKUAST=1 the time is ~1.71s.

@MasterDuke17
Remove a lot of uses of IMPL-WRAP-LIST
141bf3f 
Removing these ones doesn't break any spectests.
@MasterDuke17
UNWRAP the results of get-implicit-lookups() before indexing
2c9cfb4 
"[It] is a public API and as such can be overridden by a Raku class
which would return a List, not a VMArray. Thus we really should
IMPL-UNWRAP-LIST the result before indexing into it."
 - rakudo#5887 (comment)
@MasterDuke17 MasterDuke17 force-pushed the remove_a_lot_of_IMPL-UNWRAP-LISTs branch from 589178a to 2c9cfb4 Compare May 30, 2025 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niner niner niner requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MasterDuke17 @niner
0