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

Change implementation by trap bytecode methods with patched literals #17

Merged
merged 12 commits into from
Oct 23, 2023

Conversation

guillep
Copy link
Contributor

@guillep guillep commented Sep 6, 2023

run:with:in: is terribly slow.

Instead, use normal methods and literal patching.
This plays much better with the JIT and inline caches.

The general strategy is the following.

The original method is installed in the method dictionary with another selector (an object).
A trap method accepting the same number of arguments than the original method is copied, then it is patched so it forwards the message to the previously crafted selector.

Moreover, trap methods have been optimized to avoid allocating ensure blocks, stack reifications and other subtleties required for stack unwind (correct management of exceptions and non-local returns) which makes it very slow.
To fix this, this PR requires a change in how Pharo manages exceptions. See for example the code of ensure:

ensure: aBlock
	| complete returnValue |
	<primitive: 198>
	returnValue := self valueNoContextSwitch.
	complete ifNil:[
		complete := true.
		aBlock value.
	].
	^ returnValue

The key of the previous method is that when unwind is done, the methods with the marker 198 are found in the stack and then:

  • if the antepenultimate temp (complete) is not nil
    • the first argument is taken (the handler block) and executed via the value message
    • and the antepenultimate temp (complete) is set to true

I improved Pharo so that the handler block is instead put in the first temp, allowing the method to have as many parameters as possible.

ensure: aBlock
	| handler complete returnValue |
	<primitive: 198>
        >> handler := aBlock. <<
	returnValue := self valueNoContextSwitch.
	complete ifNil:[
		complete := true.
		aBlock value.
	].
	^ returnValue

This way, trap methods can be written as:

trapWith: arg1 with: arg2
  | handler |
  handler := "some object that understands value".
  ...

This allows

  • unwind without extra blocks and less contexts
  • having a very fast execution path without unwind in the middle.

On top of this, the handler of traps is set as a literal that is patched.
That literal is a normal object (ProxyInstrumentationDeactivator) that on value inspects the stack (we are in the slow case anyways, who cares! :D) and performs what is required to do.

This PR requires the previously explained changes into Pharo, in pharo-project/pharo#14610

@guillep guillep requested a review from Ducasse September 6, 2023 13:33
@guillep
Copy link
Contributor Author

guillep commented Sep 7, 2023

Waiting for the integration of pharo-project/pharo#14613

Copy link
Collaborator

@jordanmontt jordanmontt left a comment

Choose a reason for hiding this comment

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

The code is good and the PRs from the Pharo side are already accepted! So, for me, it's ok to merge. But, maybe before merging, we should have a release of the current commit for Pharo 11. Because when we will merge this PR MethodProxies will not work on Pharo < 12

@jordanmontt jordanmontt merged commit 4627ba2 into master Oct 23, 2023
3 checks passed
@jordanmontt jordanmontt deleted the enh/methodsasproxies branch October 23, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants