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

Instrumentation, visualization and autoscoped constructors #9452

Merged
merged 20 commits into from
Apr 3, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Mar 15, 2024

Pull Request Description

So far reproduces and later will fix #9381.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 15, 2024
@JaroslavTulach JaroslavTulach self-assigned this Mar 15, 2024
var fn = c.getConstructorFunction();
var r = invoke.execute(fn, frame, state, unresolved.args);
return r;
}
}

private void notifyInsertedWithCheck() {
Copy link
Member Author

Choose a reason for hiding this comment

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

The here-in provided changes are not 100% correct. They seem to deliver some instrumentation events, but probably there are corner cases when they don't work well. Anyway, there are some events generated, so hopefully @4e6 can investigate what to do with ProgramExecutionSupport even at current state.

@4e6
Copy link
Contributor

4e6 commented Mar 26, 2024

The issue here is that in case of expression

..Cons

first, the instrumentation sees the UnresolvedSymbol node, and reports it as an expression update, and then it sees the FunctionCallInstrumentationNode wrapper containing the method pointer information

It should be the opposite, like, for example, it is done for suspended arguments. First, the instrumentation should see the FunctionCallInstrumentationNode wrapper to capture the method pointer, and then the result of the underlying expression (the UnresolvedSymbol in this case).

@JaroslavTulach
Copy link
Member Author

... the instrumentation sees the UnresolvedSymbol node, and reports it as an expression update, and then it sees the FunctionCallInstrumentationNode wrapper containing the method pointer information. It should be the opposite ...

I am afraid I cannot revert the order easily. Let's do some pair debugging to find out what can be done. Thank you for your investigation.

@enso-bot
Copy link

enso-bot bot commented Mar 26, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-25):

Progress: Started working on the issue. Debugging the simple case with the autoscope constructor without the arguments. Debugging the execution support logic to see what happens to function call wrapper after it is captured by the instrumentation It should be finished by 2024-03-29.

Next Day: Next day I will be working on the #9452 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Mar 26, 2024

Dmitry Bushev reports a new STANDUP for today (2024-03-26):

Progress: Continue working on the issue. Debugging the case with the suspended arguments. Found out that the wrapped value should also be instrumentable. Trying to enable the instrumentation for the value wrapped in the function call instrumentation node. It should be finished by 2024-03-29.

Next Day: Next day I will be working on the #9452 task. Continue working on the task

@4e6
Copy link
Contributor

4e6 commented Mar 27, 2024

Autoscope constructor

The instrumentation of autoscope argument currently works this way

type T
    A
    B x
    C y z

main =
    a = test ..A
    a

test t:T = t

The IdExecutionInstrument sees the following calls:

onEnter (UnresolvedConstructor)
onReturnValue (UnresolvedConstructor) // sends expression update
onEnter (FunctionCallInstrumentationNode)
onReturnValue (FunctionCallInstrumentationNode) // captures method pointer

First, it enters the UnresolvedConstructor, and in the onReturnValue (UnresolvedConstructor) it sends the expression update. Then it enters the wrapper node and the onReturnValue (FunctionCallInstrumentationNode) contains the method pointer. But the expression update is already being sent.

Constructor passed as a lazy argument

Here is the updated program that instruments normal constructor call T.A being a lazy argument of the method test ~t:T = t

type T
    A
    B x
    C y z

main =
    a = test T.A
    a

test ~t:T = t

In this case the IdExecutionInstrument sees the following calls:

onEnter (ApplicationNode) // T.A
onEnter (FunctionCallInstrumentationNode) // entering the wrapper
onReturnValue (FunctionCallInstrumentationNode) // capturing the method pointer
onReturnValue (ApplicationNode) // sends expression update containing the method pointer captured earlier

In this case the instrument sees the onReturnValue (FunctionCallInstrumentationNode before the onReturnValue (ApplicationNode) and is able to capture the method pointer info before sending the expression update.

@enso-bot
Copy link

enso-bot bot commented Mar 28, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-27):

Progress: Continue working on the issue. Added the ydoc-server FFI interface for Java. Fixed vite build broken by the webpack dependency. Debugging the function call instrumentation node for the suspended arguments. It should be finished by 2024-03-29.

Next Day: Next day I will be working on the #9452 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Mar 29, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-28):

Progress: Continue working on the yjs. Replaced webpack with the Vite build. Added ydoc-server bundled artifact to the Vite output. Started working on adding conditional ffi import. It should be finished by 2024-03-29.

Next Day: Next day I will be working on the #9452 task. Continue working on the task

@enso-bot
Copy link

enso-bot bot commented Mar 30, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-29):

Progress: Continue working on the issue. Updated the ydoc-server FFI interface. Implemented the bundle assembly for the Java server. Tried to make the conditional build that would switch between java and wasm interfaces. It should be finished by 2024-03-29.

Next Day: Next day I will be working on the #9452 task. Continue working on the task

@JaroslavTulach
Copy link
Member Author

The instrumentation of autoscope argument currently works this way

@4e6, I am turning your examples into test cases in 4eb9494.

@JaroslavTulach
Copy link
Member Author

@4e6, after 1a13084 I am green on:

[info] - should send method pointer updates of constructors (24 milliseconds)
[info] - should send method pointer updates of simple_suspended_constructors (27 milliseconds)
[info] - should send method pointer updates of simple_autoscoped_constructors (19 milliseconds)
[info] - should send method pointer updates of autoscope constructors (23 milliseconds)

the only failing test is should send method pointer updates of partially applied autoscope constructors, can you take a look Dmitry? Can we chat?

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 3, 2024

Right now there are two failures:

33d8b6f addresses the RuntimeVisualizationsTest failure. 08043f6 adjusts the RuntimeServerTest expectations.

@JaroslavTulach JaroslavTulach changed the title Instrumentation, visualization and (autoscoped) constructors Instrumentation, visualization and autoscoped constructors Apr 3, 2024
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/InstrumentAutoscopedConstructor_9381 branch from 380f5d2 to 3a986f0 Compare April 3, 2024 05:40
@JaroslavTulach JaroslavTulach requested a review from 4e6 April 3, 2024 08:31
JaroslavTulach and others added 2 commits April 3, 2024 11:22
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Apr 3, 2024
@mergify mergify bot merged commit f8a546e into develop Apr 3, 2024
42 checks passed
@mergify mergify bot deleted the wip/jtulach/InstrumentAutoscopedConstructor_9381 branch April 3, 2024 12:14
@enso-bot enso-bot bot mentioned this pull request Apr 4, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversion of UnresolvedConstructor to real constructor isn't instrumented
2 participants