Skip to content

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented May 27, 2025

Pull Request Description

Checklist & TODO

Inception review has happened on May 28, 2025 and based on that following action items (related to this PR) were identified:

  • All code follows the
    Scala,
    Java,
  • Unit tests have been written where possible.
  • (Almost) no increase in enso executable size - done
  • Measure startup slow down
    • done - far less than 500ms when built into native image
    • with the current dual JVM mode the startup of the Ydoc server is slower - still less than 2s
    • the idea however is to bring it back below 0.5s by compiling the ydoc-server project into "dual native library"
  • Drop ydoc-server-nodejs source code - done
  • Drop ydoc-server-nodejs docker image - done - there still is ydoc-server-polyglot docker image, in case we need 1:1 replacement for the docker image
  • Add --ydoc-port <number> option to language-server - kinda done by Launching ydoc-server together with language-server #13178 (comment)
  • Expand project/open to provide also ydoc-port value
  • Change corepack pnpm dev:gui to use this GraalVM based Ydoc server: done
  • Ensure .AppImage & co. are OK using the embedded ydoc-server - works since 76a0265
  • Ensure cloud is OK using the embedded ydoc-server

@JaroslavTulach
Copy link
Member Author

Snapshot of memory usage of the JavaScript inside of org.enso.runner.Main process:

2MB

The Y.doc JavaScript data structures are estimated to fit into 2MB of memory.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented May 30, 2025

Update on Oct 6, 2025

@hubertp hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 30, 2025
@PabloBuchu
Copy link
Contributor

LGTM. After adjusting exposed port in Dockerfile all should be fixed. Lemme know when its merged and done in nightly so I can test in the cloud

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/YdocInBackend9862 branch from 9b70bb2 to 76a0265 Compare October 9, 2025 07:45
jsonAddress: ipWithSocketToAddress(cachedProject.languageServerJsonAddress),
binaryAddress: ipWithSocketToAddress(cachedProject.languageServerBinaryAddress),
ydocAddress: null,
ydocAddress: backend.Address('ws://localhost:5976'),
Copy link
Member Author

@JaroslavTulach JaroslavTulach Oct 9, 2025

Choose a reason for hiding this comment

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

  • before this change I had to specify the Ydoc port for unpacked AppImage manually:
  • enso/dist/ide/linux-unpacked/enso --engine.ydocUrl=http://localhost:5976
  • the AppImage connects to internal Ydoc server automatically
  • here is a proof internal Ydoc server (with WebSocket emulated via Helidon library) is used

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

The JS part LGTM

@enso-bot enso-bot bot mentioned this pull request Oct 10, 2025
3 tasks
@4e6 4e6 requested a review from jdunkerley as a code owner October 10, 2025 15:46
@4e6
Copy link
Contributor

4e6 commented Oct 10, 2025

@JaroslavTulach I pushed the commit that fixes nightly build and scheduled nigthly release to verify.

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.

The nightly looks fine

Copy link
Collaborator

@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 don't like that we have this hardcoded port copied over and over in multiple places. But it has been there before.

But I do mind the extra logs/print err we seem to add when starting YDoc Server. Approving, but please consider the changes.

@ExportMessage
boolean isMemberInvocable(String member) {
return "addPath".equals(member);
return "addPath".equals(member) || "findLibraries".equals(member) || "close".equals(member);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be just easier to read with a list and check with contains?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • consistency among various InteropLibrary messages is subject to may innovation
  • alas usually it is subject to clueless evolution (like in this case)
  • it starts simple return "addPath".equals(member); and then it gets more and more convoluted
  • you'd like a change to
  • yeah, that may be better
  • anyway consistency among various implementations spread around the code base would remain a problem

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 13, 2025

 INFO ide_ci::program::command: sbt ℹ️ [error]     org.enso.projectmanager.infrastructure.languageserver.LanguageServerGatewaySpec
 INFO ide_ci::program::command: sbt ℹ️ [error]     org.enso.projectmanager.protocol.ProjectShutdownSpec
 INFO ide_ci::program::command: sbt ℹ️ [error]     org.enso.projectmanager.protocol.ProjectManagementApiSpec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants