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

Making allocations through a dynamic variable #16749

Draft
wants to merge 4 commits into
base: Pharo13
Choose a base branch
from

Conversation

jordanmontt
Copy link
Member

@jordanmontt jordanmontt commented Jun 3, 2024

This PR introduced a DynamicVariable to make the allocations instead of directly calling basicNew. The reason behind this is that now Pharo has 3 allocator methods (new, newPinnned, and newTenured). Like described in issue #16731 there are around 154 re-definitions of the method new. This causes problems since if the user calls another allocator method for those classes that have re-defined new, the object will be not be initialized properly. Fixes #16731.

This PR changes the way allocations are made by using a DynamicVariable. Now, no matter will allocator method is called, the objects will be always be properly initialized even when re-defining the new method.

Let's take OrderedCollection as an example. Currently in Pharo, if one does:

OrderedCollection newPinned add: 1 this will raised an exception since the OrderedCollection object was not initialized because of the re-definition of new.

With this PR: doing OrderedCollection newPinned add: 1 or even OrderedCollection newTenured add: 1 works as excepted.


About performance

I ran some performance benches and this PR increases the allocation time.

"WITH allocator dynamic variable"
[ Object new ] bench. "54,359,190 iterations in 5 seconds 3 milliseconds. 10865318.809 per second"

[ OrderedCollection new ] bench. "51,635,208 iterations in 5 seconds 1 millisecond. 10324976.605 per second"

[ Array new: 10 ] bench "324,588,991 iterations in 5 seconds 1 millisecond. 64904817.237 per second"
"WITHOUT allocator dynamic variable"

[ Object new ] bench. "127,249,785 iterations in 5 seconds 2 milliseconds. 25439781.088 per second"

[ OrderedCollection new ] bench. "94,225,669 iterations in 5 seconds 1 millisecond. 18841365.527 per second"

[ Array new: 10 ] bench "457,925,147 iterations in 5 seconds 2 milliseconds. 91548410.036 per second"

So, this means that making the allocation through the DynamicVariable reduces performance:

  • Around 2.34 times slower for Object new
  • Around 1.8 times slower for OrderedCollection new
  • Around 1.4 times slower for Array new: 10

@jordanmontt jordanmontt marked this pull request as draft June 3, 2024 10:40
@jordanmontt
Copy link
Member Author

jordanmontt commented Jun 3, 2024

Another considerations:

  • This change will alter the scopes of the allocation. That means, if one allocates an OrderedCollection with newTenured, all the allocations that will be produced during the allocation of the collection will be tenured too; not only the OrderedCollection object itself.
  • This PR breaks the bootstrapping process
  • It would be nice to have some solution that only penalizes the allocations made by a different allocator (and not the ones made with basicNew). With this PR, we make all the allocations slower. One solution for this can be to change the primitive basicNew to check with allocator is used. If is the normal one, proceed normally; if it is another one (like tenured) fail the primitive and execute the fallback code

@demarey
Copy link
Contributor

demarey commented Jun 5, 2024

This is a nice change but I fear the degraded performance of the classical new will be a no go! for integration.
I agree that having a solution that only penalizes the allocations made by a different allocator would be nice. Did you give a try to see if it is possible to optimize the classical new way? If not possible, maybe this feature could be packaged separately so that only users that need the feature will be impacted by performance ?

@tesonep
Copy link
Collaborator

tesonep commented Jun 5, 2024

@jordanmontt have you measured the impact in performance?

@Ducasse
Copy link
Member

Ducasse commented Jun 5, 2024

What I was thinking is could not we have a special VM or library when we want to do such analyses?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allocating objects with other allocator methods can have unconsistent behavior
4 participants