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

Feature/instrumentation api #1

Merged
merged 2 commits into from
Feb 17, 2016
Merged

Feature/instrumentation api #1

merged 2 commits into from
Feb 17, 2016

Conversation

chumer
Copy link
Member

@chumer chumer commented Jan 17, 2016

Here it is: the new instrumentation framework. It is the result of several sessions/discussions/walkthroughs with [~mlvdv] and [~jtulach].

Reading Recommendation:

If you are not interested in the details skip over to the example uses in com.oracle.truffle.api.instrumentation.examples they should give the best idea.

For everybody else:

  1. Start reading with the package-info.java and then follow the references to explore the API.
  2. Look at the changes in PolyglotEngine on how users can instrument their applications.
  3. Look the examples in com.oracle.truffle.api.instrumentation.examples.
  4. Look at the changes to SimpleLanguage on how to see how languages can adopt it.
  5. Look at the instrumentation tests
  6. Look at the instrumentation implementation in InstrumentationHandler
  7. Look at changes to DefaultCallTarget to see how lazy instrumentation works
  8. Look at the implementation of the annotation processors: GenerateWrapperNodeProcessor and InstrumentationRegistrationProcessor.

There is also a trace mode (InstrumentationHandler#TRACE) that should make it easier to understand what is going on.

New and Noteworthy:
o) Filter expression language for source sections with tags that do all the syntax probing/synchronization automatically and language agnostically (see SourceSectionFilter, Instrumenter).
o) Everything is now lazy. Wrappers are created only for places which are actually instrumented and which were executed at least once. Wrappers are updated lazy on next/first execution and removed if not needed anymore.
o) Fixed set of instrumentation tags for guest languages to adopt.
o) Event bindings for language implementations that have special privileges to implement language features with the framework (see InstrumentationLanguage).
o) Creation/disposal of instruments in PolyglotEngine and out,err,in for instrumentations.
o) Only instrumented RootNode instances are tracked. No tracking of anything else.
o) Easy dispose of event bindings. Automatic binding dispose for instrumentations on removal.
o) Language agnostic annotation processor for generating wrappers (see @Instrumentable, SL implementation)

Specific review questions:

A) Which use cases do you have that are not yet covered in the API?
B) Which use cases do you see that are now covered which were not covered before?
C) How can the implementation/API be improved/simplified?
D) How could the naming be improved? (consistency, intuition, the hardest part :-) )
E) Do you see issues in evolving of this API?
F) How can the documentation be improved?
G) Do you have ideas for tools (try to build them)?

Open/Known Issues:

a) The Registration annotation processor does not work with incremental compiling.

Migration strategy/Next steps:

After taking your feedback into account I plan to push this through (if nobody objects). The old instrumentation framework will then be deprecated and removed later on. Old and new can live side-by-side for a while during adoption of the languages.

Thanks in advance to the reviewers.

@chrisseaton
Copy link
Contributor

These are the changes that adapt JRuby's attachment system to use this new API https://github.com/jruby/jruby/compare/truffle-new-instrumentation.

@chumer
Copy link
Member Author

chumer commented Jan 18, 2016

@chrisseaton I think your patch is outdated you need to use @Instrumentable instead of @GenerateWrapperNode in the latest revision.

@chumer chumer self-assigned this Jan 19, 2016
@chrisseaton
Copy link
Contributor

How do I give a node a tag now? I've got a field that tells me if a node is the first one on a line, and I'm calling that a statement. I used to return InstrumentationTag.STATEMENT if that field was set, but now I'm not sure where to do that.

Do I need to instead have a RubyStatementNode with a single child, just to record the fact that a node is at a statement? That'll mean larger ASTs and slower interpreter and compilation as every line will go through an extra execute.

@mlvdv
Copy link
Contributor

mlvdv commented Jan 20, 2016

@chrisseaton Yes: tags are now bound only (at "AST Engineering time") by annotating Node classes. The new Instrumentation APIs reduce functionality from the prototype in several ways, but this is the one with which I have been disagreeing most. I maintain that another (more dynamic) mechanism will have to be created to address situations like this one (and many others).

@chumer
Copy link
Member Author

chumer commented Jan 20, 2016

@chrisseaton

I've got a field that tells me if a node is the first one on a line, and I'm calling that a statement.

I am not sure this is a proper definition of a statement. Do you have the notion of statement at all in Ruby?

Do I need to instead have a RubyStatementNode with a single child, just to record the fact that a node is at a statement? That'll mean larger ASTs and slower interpreter and compilation as every line will go through an extra execute.

Right. How much slower would it really be? Can you make measurements? Are there additional cases were you would need to split classes? If it turns out its too much then we need to go back to a dynamic mechanism for tagging.

This is how SimpleLanguage does it: 5616185 . Tags are declared using annotations and are now bound to Java classes in order to solve mutability for tags so we can safely assume they are not changing per node class.

@mlvdv We discussed this several times and as long as there is no use case that really needs "dynamic" tagging we should not have it in the API.

@chrisseaton
Copy link
Contributor

Everything is just an expression in Ruby, so if I have a statement node then it's always just going to be a simple wrapper around another node. I'll do that for now and will see if it's the only complication.

@chrisseaton
Copy link
Contributor

Ruby's attachments work with the latest version of Truffle now, but not with Graal yet.

@mlvdv
Copy link
Contributor

mlvdv commented Jan 21, 2016

@mlvdv We discussed this several times and as long as there is no use case that really needs "dynamic" tagging we should not have it in the API.

I’ve been arguing for two alternate views about tag binding time. Here's a recap:

Too early (new): only at AST engineering time, only by language implementor (class annotations)

  • AST engineered for performance.
  • AST may not represent syntactic structure
  • Tag decisions sometimes depend on AST context
  • Changing tag assignment requires AST re-engineering, possible performance impact

Reasonable (prototype): at AST creation time by language implementor and other tools

  • Language implementor defines default tags
  • Tools can prototype/change tag decisions easily to configure user-visible behavior
  • Fewer restrictions on AST engineering
  • Tagging changes unlikely to require engineering change

Most flexible (prototype): Any time from AST creation onward, by language and tool implementors

  • Tools can tag for own purposes (possibly privately) to dynamically annotate source locations

}

@Before
public void setupDebugger() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be named setupProfiler()? Potential copy/paste artifact?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 6aaab06

@jtulach
Copy link
Contributor

jtulach commented Jan 26, 2016

Client Access

TCR: Need Jarda to speed up work on service lookup to simplify getting Instrumenter.

The tags

  • DEFINE isn't good name - remove it
  • actually (as discussed later) all the tags should be removed from the API

Chris: added StatementNode around ExpressionNode? Is duplicating classes a problem?
Christian: Class attribute guarantees immutability
Associate tags with SourceSection?

  • Tag on Node/SourceSection constructor/Node constructor
    Michael: the Ruby example shows that tags cannot be associated with a class

Tags are stable or not? SourceSection are immutable. Changing a node (with different SourceSection) would trigger AST rescan. Michael: SourceSection well defined, tags are not - a policy decision? SourceSection created by parser - need to inject. Jarda: Are tags in annotations OK for majority and just a problem in corner cases? Michael: annotations are just wrong.

Unproven tags vs. experiments: Jarda: restrict the API; Michael: don't restrict the users of the API.
Jarda: remove Tags completely from the API if they are 'too experiemental'!? Michael: why do we need to restrict the API right now?

Chris: tags as strings.
Christian: make it strings.
Jarda: doesn't like strings.
TCR: SourceSectionFilter will accept string (in SourceSection) - decoupling from nodes completely

Tutorial for Instrument Providers

Michael: Too complex to build an instrument. Singleton vs. InstrumentDescriptor.
Jarda: Don't complicate PolyglotEngine
TCR: Provide a documentation showing it is easy to write an instrument (even multiple sessions configurable one). Profiler?

Michael: will press-on the debugger rewrite

@smarr
Copy link
Contributor

smarr commented Jan 26, 2016

I got one more issue with the TruffleProfiler, not sure whether that is a general problem, but I think so. Could it be that this is missing Graal support?

I tried and tried and always failed to get anything from the TruffleProfiler when running with SOMns.
Turns out the issue is that it only works when running without Graal.

I guess, part of the problem is that such support would need to be in the Graal part of Truffle, i.e., the OptimizedCallTarget?

}

static Instrumentable getInstrumentable(Node node) {
Instrumentable instrumentable = node.getClass().getAnnotation(Instrumentable.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to get only the annotations of the current class. Is that intentional?
I tried to annotate an Invokable node class with InstrumentationTag.ROOT and I was surprised that this is not recognized in the Method and Primitive subclasses.

This is especially surprising since this behavior seems to be different from the TruffleDSL annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this might be solved with removing InstrumentationTag.

jkreindl pushed a commit to jkreindl/graal that referenced this pull request Sep 18, 2019
…cerning:

Move actions from Parser/ATG to NodeFactory
Restructured DebugExprVarNode s.t. a type of a variable is already known
while building the AST
chrisseaton referenced this pull request in Shopify/graal Nov 1, 2019
Implement .p2align as a no-op
graalvmbot pushed a commit that referenced this pull request Nov 29, 2019
Move actions from Parser/ATG to NodeFactory
Restructured DebugExprVarNode s.t. a type of a variable is already known
while building the AST
graalvmbot pushed a commit that referenced this pull request Jul 8, 2020
* Before:
  VMThread 000000000afdc2f0  STATUS_IN_SAFEPOINT
  VMThread 00007f3e6c000b60  STATUS_IN_SAFEPOINT
* After:
  "main" #1 tid=0x000000000ab2b2f0 state=TIMED_WAITING
  "nfi-gc" #4 daemon tid=0x00007f9dec000b60 state=WAITING
* HotSpot:
  "main" #1 prio=5 os_prio=0 tid=0x00007fac2400a800 nid=0x4c2f waiting on condition [0x00007fac2b591000]
  "nfi-gc" #20 daemon prio=5 os_prio=0 tid=0x00007f548c472800 nid=0x74a9 in Object.wait() [0x00007f5469518000]
* Omit the VMThread status since it's always STATUS_IN_SAFEPOINT.
tofische referenced this pull request in gwipplinger/graal May 31, 2021
@edrobal edrobal mentioned this pull request May 18, 2022
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.

9 participants