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

Use CompilerDirectives::castExact instead of ValueProfiles #256

Merged
merged 3 commits into from Sep 4, 2018

Conversation

2 participants
@fniephaus
Copy link
Contributor

commented Jul 10, 2018

The assumption is that performance should remain the same (benchmarks needed for verification). At the same time, this change would simplify code.

Closes #255

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Turns out the Truffle version that SOMns is using, does not implement castExact yet.

@smarr

This comment has been minimized.

Copy link
Owner

commented Jul 10, 2018

:( damn, yeah, I need to update it, but that's going to take time, which I currently don't have.

@smarr

smarr approved these changes Jul 10, 2018

Copy link
Owner

left a comment

Thanks for the PR!

The changes look good to me.
But yeah, I won't be able to merge this before updating Truffle.

And, updating Truffle means I need to update my Truffle changes, adapt the changes for the instrumentation, etc. Last time I tried doing that, I did not manage to get everything working again. So, will take more time.

@smarr smarr added the enhancement label Jul 10, 2018

@smarr smarr self-assigned this Jul 10, 2018

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

No worries, feel free to merge this when done updating Truffle :)
BTW: I've adjusted GraalSqueak and haven't noticed any significant change in performance.

@smarr

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2018

@fniephaus Hm, I guess we could do an in-between change, but not sure this is super useful at the moment.

@smarr

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2018

And for the record, the CompilerDirectives.castExact(storage, int.class); is not correct, because we have an object. Therefore, it should be CompilerDirectives.castExact(storage, Integer.class); (as seen in the gitter chat)

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

Why shouldn't this be useful at the moment? Performance should remain the same, code is simpler? castExact will only help to remove the remaining ValueProfile. Also, we don't need to it for the plain Integer case, A cast to an unboxed int should do the trick, right?

fniephaus added some commits Jul 10, 2018

Use CompilerDirectives::castExact
instead of ValueProfiles. The assumption is that performance should remain the same (benchmarks needed for verification). At the same time, this change would simplify code.

Closes #255
Use direct casts instead of `exactCast`
According to @woess, `CompilerDirectives::castExact` is only needed if the target class is not final. Since primitive types as well as PartiallyEmptyArrays are final, the `storage` field can be casted directly. This is not the case for Object[], so use a single ValueProfile until SOMns' Truffle version supports `castExact`.

@smarr smarr force-pushed the fniephaus:cast-exact branch from 4f6832a to fcdc786 Sep 3, 2018

Apply todo, and solve a few compilation issues
Signed-off-by: Stefan Marr <git@stefan-marr.de>

@smarr smarr force-pushed the fniephaus:cast-exact branch from fcdc786 to 75b3595 Sep 4, 2018

@smarr smarr added this to the v0.7.0 milestone Sep 4, 2018

@smarr smarr added this to Open Issues in Completeness via automation Sep 4, 2018

@smarr

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2018

I fixed a couple of compilation issues, and applied the exactCast. Tests are passing, but still waiting for the benchmark results.

@smarr

This comment has been minimized.

Copy link
Owner

commented Sep 4, 2018

Results are very noisy. Ups and downs, on peak performance as well as interpreter speed.
Think the measurements aren't very reliable.
Though, they don't indicate a performance problem.
So, I'll merge this since it simplifies the code.

@smarr smarr merged commit e7c95a9 into smarr:dev Sep 4, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Completeness automation moved this from Open Issues to Completed Sep 4, 2018

@fniephaus fniephaus deleted the fniephaus:cast-exact branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.