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

[Compatibility] Add Module#const_added #3099

Closed

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented Jun 7, 2023

Source: #3039

Module#const_added has been added. [Feature #17881]

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 7, 2023
@itarato itarato self-assigned this Jun 7, 2023
@itarato itarato added the shopify label Jun 7, 2023
@itarato itarato changed the title [Compatibility] Add Method#const_addeed [WIP] [Compatibility] Add Method#const_addeed Jun 8, 2023
@itarato itarato force-pushed the feature/PA-Add-module-const-added branch 2 times, most recently from 20f253f to da022c8 Compare June 9, 2023 14:53
@itarato itarato marked this pull request as ready for review June 9, 2023 17:02
@itarato itarato changed the title [WIP] [Compatibility] Add Method#const_addeed [Compatibility] Add Method#const_addeed Jun 9, 2023
@andrykonchin andrykonchin changed the title [Compatibility] Add Method#const_addeed [Compatibility] Add Method#const_added Jun 12, 2023
@itarato itarato force-pushed the feature/PA-Add-module-const-added branch from da022c8 to 8928843 Compare June 12, 2023 15:25
@andrykonchin andrykonchin self-assigned this Jul 27, 2023
@andrykonchin andrykonchin changed the title [Compatibility] Add Method#const_added [Compatibility] Add Module#const_added Jul 27, 2023
@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Jul 27, 2023
@eregon
Copy link
Member

eregon commented Jul 28, 2023

I worry that this feature, which causes an extra method call for every constant assignment, can significantly slow down loading code.
I'm thinking we could optimize e.g. by having a global boolean constAddedDefined in RubyContext, and only call if there ever was a definition of const_added (not counting the empty core one).

Also we'd be able to remove the currentNode != null && coreLibrary != null && coreLibrary.isLoaded() check and just use that boolean instead which seems nice.
Maybe @andrykonchin can do that to speed up the integration of this PR?

Longer-term when we have vtable for methods we could use that to quickly check if const_added on a given module is the core one and in that case do nothing.

@itarato
Copy link
Collaborator Author

itarato commented Jul 29, 2023

I worry that this feature, which causes an extra method call for every constant assignment, can significantly slow down loading code. I'm thinking we could optimize e.g. by having a global boolean constAddedDefined in RubyContext, and only call if there ever was a definition of const_added (not counting the empty core one).

Also we'd be able to remove the currentNode != null && coreLibrary != null && coreLibrary.isLoaded() check and just use that boolean instead which seems nice. Maybe @andrykonchin can do that to speed up the integration of this PR?

Longer-term when we have vtable for methods we could use that to quickly check if const_added on a given module is the core one and in that case do nothing.

Sure thing, I let you make that change. Thanks!

@andrykonchin
Copy link
Member

Merged in cd498f7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants