Blockly: Create and Delete Events Generate Lots of Detached Nodes

[BeksOmega]

Describe

Create and delete events generate lots of detached nodes.

This is because those events store the xml of the created/deleted blocks.

To Observe

Steps to observe the behavior:

1. Go to the playground.
2. Run the spaghetti stress test.
3. Delete all of the blocks.
4. Create a heap snapshot.
   A) Go to the memory tab of the chrome dev tools.
   B) Select the Heap Snapshot button.
   C) Press the "Take Snapshot" button.
   D) In the "Class filter" type in "Detached"
5. Observe the large number of detached nodes:
   
6. Expand the detached node, and select the first entry.
7. Observe how it is part of a delete event:
   

Describe the solution you'd like

Honestly, I know that detached nodes are bad but I don't know what you're supposed to do instead, since these are being used for storage purposes.

Maybe storing the block information as JSON would be better? I have designed a system for adding JSON serialization to the Blockly serializer.

Describe alternatives you've considered

Maybe converting the dom to text would be better? But again I really have no knowledge in this area.

Additional context

Tracking issue requested by Rachel.

[rachel-fenichel]

These are being kept alive by the undo stack on the workspace--calling Blockly.mainWorkspace.clearUndo() in the playground clears them out. A reasonable solution is to set a depth on the undo stack. 64 undos ought to be enough for anybody.

[rachel-fenichel]

@picklesrus @ewpatton Do you folks already have a depth on the undo queue to solve this?

[ewpatton]

We do not set a limit in App Inventor.

It would be interesting to do some performance tests to see how much it affects us, but based on previous experience it’s the layout and render passes over many blocks that tend to make our workspace bog down once N is large. I’d be interested in seeing how that ends up changing with the new rendering pipeline.

Sent from my iPhone

On Sep 10, 2019, at 18:33, Rachel Fenichel [e-mail] wrote:

@picklesrus @ewpatton Do you folks already have a depth on the undo queue to solve this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[rachel-fenichel]

I think this would matter if you have a long-lived workspace, like a tab you've had open for a lot of time and have been poking at. Maybe also when opening a giant workspace, since that would create these all at once, so that could be a good way to check.

[ewpatton]

Long lived workspaces, possibly, but it’s not yet clear exactly how much memory is “leaking” there. Since we don’t record events during load, there shouldn’t be anything on the undo stack at the start of a session even for large projects. I can run some tests and get concrete numbers on Thursday.

Sent from my iPhone

On Sep 10, 2019, at 19:50, Rachel Fenichel [e-mail] wrote:

I think this would matter if you have a long-lived workspace, like a tab you've had open for a lot of time and have been poking at. Maybe also when opening a giant workspace, since that would create these all at once, so that could be a good way to check.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[NeilFraser]

I ran some tests in Chrome. My results were fairly consistent between runs, so I have confidence in the results.

• Load playground, memory snapshot: 5.3 MB.
• Run Spaghetti test, memory snapshot: 21.2 MB.
• Delete playground, wait a minute for garbage collection, memory snapshot: 5.7 MB.

Now comment out the two lines where oldXml is set and repeat:

• Delete playground, wait a minute for garbage collection, memory snapshot: 5.6 MB.

This indicates that the XML (the detached Nodes) takes 100 KB. By comparison, converting this XML to text:

Blockly.Xml.domToText(workspace.undoStack_[2].oldXml).length/1024 -> 326

My conclusions are:
1) XML nodes are being stored three times more efficiently than the easiest text-based representation.
2) The leak of about 33 bytes per block deletion (100 KB for 3,000 blocks) seems low enough not to worry about, since it serves a genuine purpose (undo).

[NeilFraser]

I should follow up and say that this is absolutely a topic that should be explored. There may be monsters lurking in our memory management that we aren't aware of. Like that time when we discovered that the connection database was O(n^2) and was using the majority of our CPU time due to a trivial inefficiency.

There are not a lot of eyes on our memory heap, so it's a promising thing to poke at. Thank you @BeksOmega!

[rachel-fenichel]

Turns out we have a MAX_UNDO, so this is bounded (as well as being small per-block). Closing.