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.

Sign up for GitHub

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

Jump to bottom

Avoid whole source reparsing when the IDE performs a simple edit #3508

Merged
merged 67 commits into from
Jun 16, 2022
Merged

Avoid whole source reparsing when the IDE performs a simple edit #3508

merged 67 commits into from
Jun 16, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jun 3, 2022 •
edited
Loading

Pull Request Description

The goal of this PR is to demonstrate we can avoid parsing of the whole program when the IDE makes simple edit. The PR contains new [IncrementalUpdatesTest](https://github.com/enso-org/enso/compare/wip/jtulach/IncrementalUpdates-182323784?expand=1#diff-225738cc33a66aea979256736e4467473c6d5a4e0b16efe821caac3c1f4730c1) which (via NodeCountingTestInstrument) checks whether a new node is created or not. Right now the test fails with an error 28 nodes created instead of none when replacing 4 with 5:

No new nodes created. Maximal size should be 0, but was: 28 in
[error] org.enso.interpreter.node.expression.constant.DynamicSymbolNode @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.expression.constant.DynamicSymbolNode @ Enso_Test.Test.Main:9
[error] org.enso.interpreter.node.expression.literal.IntegerLiteralNode @ Enso_Test.Test.Main:4
[error] org.enso.interpreter.node.callable.ApplicationNode @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.callable.ApplicationNode @ Enso_Test.Test.Main:9
[error] org.enso.interpreter.node.callable.thunk.ForceNodeGen @ Enso_Test.Test.Main:5
[error] org.enso.interpreter.node.callable.argument.ReadArgumentNode
[error] org.enso.interpreter.node.callable.argument.ReadArgumentNode
[error] org.enso.interpreter.node.callable.function.StatementNode @ Enso_Test.Test.Main:7
[error] org.enso.interpreter.node.callable.function.StatementNode @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.callable.function.StatementNode @ Enso_Test.Test.Main:3
[error] org.enso.interpreter.node.callable.function.StatementNode @ Enso_Test.Test.Main:4
[error] org.enso.interpreter.node.callable.function.StatementNode
[error] org.enso.interpreter.node.callable.function.StatementNode
[error] org.enso.interpreter.node.callable.function.BlockNode
[error] org.enso.interpreter.node.callable.function.BlockNode @ Enso_Test.Test.Main:7
[error] org.enso.interpreter.node.callable.function.BlockNode
[error] org.enso.interpreter.node.callable.function.BlockNode @ Enso_Test.Test.Main:3
[error] org.enso.interpreter.node.callable.FunctionCallInstrumentationNode @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.callable.FunctionCallInstrumentationNode @ Enso_Test.Test.Main:9
[error] org.enso.interpreter.node.expression.constant.ConstructorNodeGen @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.expression.constant.ConstructorNodeGen @ Enso_Test.Test.Main:9
[error] org.enso.interpreter.node.scope.ReadLocalVariableNodeGen @ Enso_Test.Test.Main:5
[error] org.enso.interpreter.node.scope.ReadLocalVariableNodeGen @ Enso_Test.Test.Main:9
[error] org.enso.interpreter.node.scope.AssignmentNodeGen
[error] org.enso.interpreter.node.scope.AssignmentNodeGen @ Enso_Test.Test.Main:8
[error] org.enso.interpreter.node.scope.AssignmentNodeGen
[error] org.enso.interpreter.node.scope.AssignmentNodeGen @ Enso_Test.Test.Main:4, took 5.622 sec
[error] Failed: Total 1, Failed 1, Errors 0, Passed 0

The 8bd50ab commit fixes the test by finding the right IntegerLiteralNode in the Truffle AST and updating it.

Important Notes

Checklist

Please include the following checklist in your PR:

  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner June 3, 2022 08:16
@JaroslavTulach JaroslavTulach marked this pull request as draft June 3, 2022 08:16
kustosz
kustosz reviewed Jun 3, 2022
View reviewed changes
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Quite elegant, though we definitely should update the IR as well. Also a few remarks inline

...e/runtime/src/main/java/org/enso/interpreter/node/expression/literal/IntegerLiteralNode.java Outdated Show resolved Hide resolved
...e/runtime/src/main/java/org/enso/interpreter/node/expression/literal/IntegerLiteralNode.java Outdated Show resolved Hide resolved
@JaroslavTulach JaroslavTulach self-assigned this Jun 5, 2022
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/IncrementalUpdates-182323784 branch from ff44a4d to 1164679 Compare June 7, 2022 11:46
kustosz
kustosz reviewed Jun 8, 2022
View reviewed changes
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Logically this seems right, some comments left inline.

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/PatchedModuleValues.java Outdated Show resolved Hide resolved
4e6
4e6 requested changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

Nicely done! Please clean things up a bit before we merge it

...rument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java Show resolved Hide resolved
...ne/runtime/src/main/java/org/enso/interpreter/node/callable/function/CreateFunctionNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
JaroslavTulach and others added 4 commits June 16, 2022 11:11
@JaroslavTulach
Compute simpleUpdate with the help of map and filter
ba152bc
@JaroslavTulach @4e6
Javadoc clarification
08dbd6f 
Co-authored-by: Dmitry Bushev <bushevdv@gmail.com>
@JaroslavTulach @4e6
Javadoc clarification
c8f0228 
Co-authored-by: Dmitry Bushev <bushevdv@gmail.com>
@JaroslavTulach
Addressing another round of comments
6c81820
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jun 16, 2022 •
edited
Loading

Commit 6c81820 addressed the latest comments. I am not sure what to do about #3508 (comment) - as there already is suggested small change in a single literal or {@code null} which describes what the parameter is used for.

Adding "CI: Ready to merge" label. Awaiting approval.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jun 16, 2022
4e6
4e6 approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

A few more comments by me, but overall looks good now!

...rument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/PatchedModuleValues.java Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala Show resolved Hide resolved
@4e6 4e6 removed the CI: Ready to merge This PR is eligible for automatic merge label Jun 16, 2022
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/IncrementalUpdates-182323784 branch from 574625c to 52630ed Compare June 16, 2022 10:00
hubertp
hubertp approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I think we let you suffer enough of Scala, just make sure numericValue returns Either and it lgtm

engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/context/ChangesetBuilder.scala Outdated Show resolved Hide resolved
JaroslavTulach and others added 8 commits June 16, 2022 12:15
@JaroslavTulach
More information in Javadoc
5d280d5
@JaroslavTulach @hubertp
newIR.map
0a573ed 
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
@JaroslavTulach @hubertp
newIR.map 2
ec417c8 
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
@JaroslavTulach @hubertp
Underscore
f088163 
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
@JaroslavTulach @hubertp
_._2.size
f54d387 
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
@JaroslavTulach
Ensure only IR.Literal is returned
7144dbc
@JaroslavTulach
Throw CompilerError rather than returning a string with error message
a5db906
@JaroslavTulach
Merge branch 'develop' into wip/jtulach/IncrementalUpdates-182323784
68a9edf
@JaroslavTulach JaroslavTulach merged commit c72a658 into develop Jun 16, 2022
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/IncrementalUpdates-182323784 branch June 16, 2022 14:02
kazcw pushed a commit that referenced this pull request Jun 29, 2022
@JaroslavTulach @4e6
@hubertp @kazcw
Avoid whole source reparsing when the IDE performs a simple edit (#3508)
285b6d5 
Co-authored-by: Dmitry Bushev <bushevdv@gmail.com>
Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
This was referenced Feb 6, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@4e6 4e6 4e6 approved these changes

@kustosz kustosz kustosz approved these changes

@ekmett ekmett Awaiting requested review from ekmett

Assignees

@JaroslavTulach JaroslavTulach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JaroslavTulach @hubertp @4e6 @kustosz