Drop method exported from WASM + removing leaks. #6365

farmaazon · 2023-04-20T10:12:13Z

Pull Request Description

The drop method is available in the WASM object. This can be tested by typing ensoglApp.wasm.drop() in the dev console - all objects should be removed and all connections closed.

Important Notes

This PR fixed serveral leaks by this occasion
A new tool for tracking leaks was added to prelude's debug module.

Checklist

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

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

lib/rust/prelude/src/debug.rs

PabloBuchu · 2023-04-20T15:46:40Z

Hej @farmaazon thank you for this. Unfortunately this solution requires wasm to be fully loaded and initialised which is not cloud case.

Our current flow is:
We initialise ensogl App and pass instance reference to authentication module which takes care of setting user session. Here for now with newDashboard flag set to false you can run old IDE which calls .run which loads and initialise wasm..

New dashboard reinitialise ensoglApp on project run, sets startup.project and then calls ensoglApp.run. So unfortunately we don't have access to ensoglApp.wasm

Let me know what time suits you best tomorrow for a call so we can check whats are our options :)

wdanilo · 2023-04-20T16:14:25Z

But ensoglApp.run initializes WASM so ... we have access to what Adam exposed I think. Or I don't understand something. anyway, add me to the call tomorrow, please.

vitvakatu · 2023-04-20T16:34:26Z

@PabloBuchu @wdanilo fyi, Adam is off tomorrow.

wdanilo · 2023-04-20T17:54:59Z

@vitvakatu thanks a lot! In the meantime @PabloBuchu has written to me that this might be resolved. @PabloBuchu am I right here?

sylwiabr · 2023-04-21T07:29:58Z

can we merge this?who is doing QA here?

PabloBuchu · 2023-04-21T07:53:41Z

@sylwiabr @wdanilo

QA 🟢 we should merge this so in cloud we could use the drop method to switch between project

Thank you for quickly delivering this :)

somebody1234 · 2023-04-24T07:04:25Z

not sure if it's out of scope for this PR, but .drop() should probably remove the root element as well?

farmaazon · 2023-04-24T08:15:25Z

can we merge this?who is doing QA here?

First, it should be reviewed. I've got no accept from anyone. @wdanilo @MichaelMauderer can you review it?

wdanilo · 2023-04-24T08:58:59Z

I'll review it in 1h!

vitvakatu · 2023-04-24T09:14:14Z

not sure if it's out of scope for this PR, but .drop() should probably remove the root element as well?

We're not creating it (it is hardcoded in the HTML), so I don't think we should remove it.

somebody1234 · 2023-04-24T09:18:50Z

oh sorry brainfart, i meant the scene element - otherwise the new scene element is hidden under the old one so it just looks blank

MichaelMauderer · 2023-04-24T08:33:57Z

app/gui/src/lib.rs

+        let objects = objects.borrow();
+        if !objects.is_empty() {
+            error!("Tracked objects leaked after dropping entire application!");
+            error!("{objects:#?}");


Should this message be prefaced with some info? Maybe error!("Objects that go leaked: {objects:#?}");

MichaelMauderer · 2023-04-24T09:32:09Z

lib/rust/prelude/src/debug.rs

+
+
+
+// ====================


Should this be in the prelude, or is there a better place for it closer to the executor or other memory management functionality?

It needs to be available from many places, including both IDE controllers and view. Also, the executor I know is more about handling asynchronous tasks, not memory management.

I cannot think about any other package.

wdanilo · 2023-04-24T11:15:56Z

lib/rust/prelude/src/debug.rs

+// === LeakDetector ===
+// ====================
+
+/// A module containing an utility for detecting leaks.


I was reading documentation of this module and the inner structs and I got the high-level overview, but there was no examples how this should be used and no explanation of how to use it. The constructor function documentation Create enabled structure with appointed entity name (shared between all copies). is not helpful either - I mean, it creates an "enabled" structure with a shared name, but then what? I think I'd need a little bit more docs here.

Also, I don't see how it's used in the new code, but that's probably it is somehow connected to the existing TraceCopies and I need some docs to understand it better.

Create enabled structure with appointed entity name (shared between all copies). is not helpful either - I mean, it creates an "enabled" structure with a shared name, but then what? I think I'd need a little bit more docs here.

Well, then it does exactly what struct's documentation says:

The underlying TraceCopies tracks copies, printing when they are created and dropped.

Additionally, the existing copies are stored in [TRACKED_OBJECTS].
The documentation should be read with TraceCopies the lead_detector::Trace is a wrapper over it (and documentation says it).

What information do you still miss?

Ok, I think adding a clear and explicit information that all the Traces are stored in TRACKED_OBJECTS when the leak detector is enabled is good enough, but I missed that from docs. Maybe someone else would find it obvious, but I don't. I'd be very thankful for adding a line with this explanation there.

…-drop-6317

vitvakatu · 2023-04-24T19:31:35Z

QA Report 🟡

@somebody1234 noted that the scene element is left when we drop the application. We're creating a bunch of elements, so we should probably remove them. I don't know if it's necessary for cloud needs.

These elements are left after drop:

Otherwise looks good.

somebody1234 · 2023-04-25T02:30:00Z

it's easy enough to remove on the cloud side - however i think here is the right place to handle it

also note that all the other elements are children of the scene element so just removing the scene element should be enough

…-drop-6317

PabloBuchu · 2023-04-25T09:07:22Z

@farmaazon are you able to add removing div.scene element from DOM?

farmaazon · 2023-04-25T13:59:39Z

@farmaazon are you able to add removing div.scene element from DOM?

Already there. Unfortunately, I spent some time on it trying to discover more leaks. Wasn't successful, but applied a workaround.

vitvakatu · 2023-04-25T14:16:46Z

QA report 🟢

Looks good to me, the only thing is that there are a bunch of warnings in the console:

But I guess this can be addressed later.

somebody1234 · 2023-04-25T15:04:22Z

seems like ensogl is still running after the element is gone -
can confirm i have been experiencing that on the new dashboard where this is used

re: after the element is gone, the same error message (zero size) has happened before when i set the display to none accidentally (which hides the element, making getBoundingClientRect() return a rectangle with zero size). not sure if that helps though

Fixes #6317 The `drop` method is available in the WASM object. This can be tested by typing `ensoglApp.wasm.drop()` in the dev console - all objects should be removed and all connections closed. # Important Notes * This PR fixed serveral leaks by this occasion * A new tool for tracking leaks was added to prelude's `debug` module.

* develop: Passing events to sub-widgets in List Editor and refactoring of the slider component. (#6433) Revert "Cloud/desktop mode switcher (#6308)" (#6444) Widgets integrated with graph nodes (#6347) Table Visualization and display text changes. (#6382) Skip redundant compilations (#6436) Add parse extensions to Text type. #6330 (#6404) Cloud/desktop mode switcher (#6308) Replace Table should_equal with should_equal_verbose (#6405) Rollback event handling changes for the mouse (#6396) Dashboard directory interactivity (#6279) Ability to change the execution environment between design and live. (#6341) Implementation of elements adding to List Editor and a lot of internal API (#6390) Drop method exported from WASM + removing leaks. (#6365) Turn null into UnexpectedExpression when Union type is incomplete (#6415)

farmaazon added 3 commits April 20, 2023 12:09

Graph Editor leaks no more

098f215

Fixed all leaks, but code needs to be cleaned up

03a8e4e

Cleanup

cc2d78d

farmaazon added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 20, 2023

farmaazon requested a review from MichaelMauderer as a code owner April 20, 2023 10:12

farmaazon self-assigned this Apr 20, 2023

farmaazon requested review from wdanilo, kazcw and mwu-tow as code owners April 20, 2023 10:12

vitvakatu reviewed Apr 20, 2023

View reviewed changes

lib/rust/prelude/src/debug.rs Outdated Show resolved Hide resolved

Apply @vitvakatu review

db4b6e7

somebody1234 mentioned this pull request Apr 24, 2023

Cloud/desktop mode switcher #6308

Merged

5 tasks

MichaelMauderer approved these changes Apr 24, 2023

View reviewed changes

wdanilo approved these changes Apr 24, 2023

View reviewed changes

farmaazon added 4 commits April 24, 2023 14:15

Merge remote-tracking branch 'origin/develop' into wip/farmaazon/wasm…

dd75dde

…-drop-6317

Extend the documentation a bit

d732d28

Extend docs even more and fix compilation after merge

72a10cf

Extend one log

574de9e

Merge remote-tracking branch 'origin/develop' into wip/farmaazon/wasm…

8a58f21

…-drop-6317

farmaazon added 2 commits April 25, 2023 15:51

Remove DOM elements

c65c100

Fix compilation

85423ba

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 25, 2023

mergify bot merged commit 952de24 into develop Apr 25, 2023

mergify bot deleted the wip/farmaazon/wasm-drop-6317 branch April 25, 2023 15:38

This was referenced Apr 27, 2023

Cloud/desktop mode switcher #6448

Merged

Using wasm drop method results in warnings flood #6505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop method exported from WASM + removing leaks. #6365

Drop method exported from WASM + removing leaks. #6365

farmaazon commented Apr 20, 2023

PabloBuchu commented Apr 20, 2023

wdanilo commented Apr 20, 2023

vitvakatu commented Apr 20, 2023

wdanilo commented Apr 20, 2023

sylwiabr commented Apr 21, 2023

PabloBuchu commented Apr 21, 2023

somebody1234 commented Apr 24, 2023

farmaazon commented Apr 24, 2023

wdanilo commented Apr 24, 2023

vitvakatu commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

MichaelMauderer Apr 24, 2023

MichaelMauderer Apr 24, 2023

farmaazon Apr 24, 2023

wdanilo Apr 24, 2023

farmaazon Apr 24, 2023

wdanilo Apr 24, 2023

vitvakatu commented Apr 24, 2023

somebody1234 commented Apr 25, 2023

PabloBuchu commented Apr 25, 2023

farmaazon commented Apr 25, 2023

vitvakatu commented Apr 25, 2023

somebody1234 commented Apr 25, 2023

Drop method exported from WASM + removing leaks. #6365

Drop method exported from WASM + removing leaks. #6365

Conversation

farmaazon commented Apr 20, 2023

Pull Request Description

Important Notes

Checklist

PabloBuchu commented Apr 20, 2023

wdanilo commented Apr 20, 2023

vitvakatu commented Apr 20, 2023

wdanilo commented Apr 20, 2023

sylwiabr commented Apr 21, 2023

PabloBuchu commented Apr 21, 2023

somebody1234 commented Apr 24, 2023

farmaazon commented Apr 24, 2023

wdanilo commented Apr 24, 2023

vitvakatu commented Apr 24, 2023

somebody1234 commented Apr 24, 2023

MichaelMauderer Apr 24, 2023

Choose a reason for hiding this comment

MichaelMauderer Apr 24, 2023

Choose a reason for hiding this comment

farmaazon Apr 24, 2023

Choose a reason for hiding this comment

wdanilo Apr 24, 2023

Choose a reason for hiding this comment

farmaazon Apr 24, 2023

Choose a reason for hiding this comment

wdanilo Apr 24, 2023

Choose a reason for hiding this comment

vitvakatu commented Apr 24, 2023

QA Report 🟡

somebody1234 commented Apr 25, 2023

PabloBuchu commented Apr 25, 2023

farmaazon commented Apr 25, 2023

vitvakatu commented Apr 25, 2023

QA report 🟢

somebody1234 commented Apr 25, 2023