search

oracle / graal

GraalVM compiles Java applications into native executables that start instantly, scale fast, and use fewer compute resources 🚀
https://www.graalvm.org
Other
20.28k stars 1.63k forks source link

Instrument `onReturnX` being called but not `onEnter` #2772

Open chrisseaton opened 4 years ago

chrisseaton commented 4 years ago

I've pushed one commit on top of the Thermometer branch.

https://github.com/Shopify/graal/tree/thermometer-restore

The thermometer sets a flag in onEnter. I was trying to do something where I would save the old value and restore it on onReturnX.

https://github.com/Shopify/graal/blob/thermometer-restore/tools/src/com.oracle.truffle.tools.thermometer/src/com/oracle/truffle/tools/thermometer/ThermometerCompilationSampleNode.java#L70-L72

I gave up doing that as the overhead was too high.

However - you'll see I had to deal with onReturnX being called but not onEnter. @chumer says that this isn't intended behaviour and we think it's most likely an issue with lazy nodes or the special wrapper for defined nodes.

If you set a breakpoint in the constructor of ThermometerCompilationSampleNode you'll get an idea of what could be going wrong.

The event node factory is correctly installed before nodes are executed.

eregon commented 4 years ago

What's the status of this? @chrisseaton If you think it's an issue in Ruby in TruffleRuby then can I assign you? If you think it's a Truffle issue, then I'll move it to that repo. But given that the branch does not exist anymore, maybe we can just close this issue since it's not a problem in practice for you? I'll optimistically do that.

Given that instrumentation can happen in the middle of a method, I think there might be nodes for which onEnter() will not be called (it would have to be retroactive). Lazy nodes amplify that, because even if instrumentation was already enabled it would dynamically add new nodes. Those new nodes should be instrumented before execution though. We use notifyInserted() in LazyRubyNode since 2017, so it seems rather unlikely to be a problem in TruffleRuby.

chrisseaton commented 4 years ago

It's not an issue for me - I'm working around:

I gave up doing that as the overhead was too high.

And the problem isn't as simple as onEnter not being called for nodes already on the stack.

I'm not sure why you've closed though - doesn't everyone agree this is a bug? Should we move it to the Truffle repo instead?

eregon commented 4 years ago

OK I moved it as a Truffle issue and reopened it. I don't know if it should be considered a bug or not, @chumer?