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

Coloring exhibits quadratic behavior and blocks the SWT thread #540

Closed
szarnekow opened this issue Jun 7, 2023 · 15 comments
Closed

Coloring exhibits quadratic behavior and blocks the SWT thread #540

szarnekow opened this issue Jun 7, 2023 · 15 comments
Assignees

Comments

@szarnekow
Copy link

szarnekow commented Jun 7, 2023

Opening a medium[1] sized JSON file in Eclipse might block the UI seemingly indefinitely due to AbstractModelLines.getOrNull which attempts query a linked list by index.

Relevant part of the stack:

"main" #1 [259] prio=6 os_prio=31 cpu=255346.03ms elapsed=871.90s tid=0x00007fadbe021400 nid=259 runnable  [0x0000000205a14000]
   java.lang.Thread.State: RUNNABLE
	at java.util.LinkedList.node(java.base@20.0.1/LinkedList.java:575)
	at java.util.LinkedList.get(java.base@20.0.1/LinkedList.java:481)
	at org.eclipse.tm4e.core.model.AbstractModelLines.getOrNull(AbstractModelLines.java:149)
	- locked <0x0000000769f00080> (a java.util.LinkedList)
	at org.eclipse.tm4e.core.model.TMModel.getLineTokens(TMModel.java:284)
	at org.eclipse.tm4e.ui.text.TMPresentationReconciler.colorize(TMPresentationReconciler.java:540)
	at org.eclipse.tm4e.ui.text.TMPresentationReconciler$InternalListener.textChanged(TMPresentationReconciler.java:336)
	at org.eclipse.jface.text.TextViewer.updateTextListeners(TextViewer.java:2785)
	at org.eclipse.jface.text.TextViewer.invalidateTextPresentation(TextViewer.java:3405)
	at org.eclipse.jface.text.TextViewer.initializeWidgetContents(TextViewer.java:3453)
	at org.eclipse.jface.text.TextViewer.setVisibleDocument(TextViewer.java:3492)

[1] To quantify this: 800k lines, 22MB; we might call it a big JSON file.

It shall be noted that a quick glance over AbstractModelLines.java indicates that almost all of the usages of the internal linked list are index based. It might warrant a refactoring of the entire class to avoid performance penalty that is imposed on all methods with the current data structure.

@mickaelistria
Copy link
Contributor

What can be a good remediation? Just switching to ArrayList? Freel free to submit PRs for possible improvements.

@sebthom
Copy link
Member

sebthom commented Jun 7, 2023

I am investigating that.

sebthom added a commit to sebthom/tm4e that referenced this issue Jun 7, 2023
sebthom added a commit to sebthom/tm4e that referenced this issue Jun 7, 2023
sebthom added a commit to sebthom/tm4e that referenced this issue Jun 8, 2023
sebthom added a commit to sebthom/tm4e that referenced this issue Jun 8, 2023
sebthom added a commit to sebthom/tm4e that referenced this issue Jun 9, 2023
@sebthom
Copy link
Member

sebthom commented Jun 9, 2023

Please try the latest snapshot from https://download.eclipse.org/tm4e/snapshots/ it contains the performance improvements I merged.

@sebthom
Copy link
Member

sebthom commented Jun 17, 2023

Issue has been solved in latest snapshot build (confirmed by other parties).

@sebthom sebthom closed this as completed Jun 17, 2023
@szarnekow
Copy link
Author

@sebthom I installed the latest nightly snapshot and still see a lot of time is spent here:

"main" #1 [259] prio=6 os_prio=31 cpu=141396.40ms elapsed=533.04s tid=0x00007fa2ed017000 nid=259 runnable  [0x0000000209d21000]
   java.lang.Thread.State: RUNNABLE
	at org.eclipse.tm4e.core.model.AbstractModelLines.replaceLines(AbstractModelLines.java:118)
	- locked <0x0000000758f6b1a0> (a java.util.ArrayList)
	at org.eclipse.tm4e.ui.internal.model.DocumentModelLines.documentChanged(DocumentModelLines.java:72)
	at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged2(AbstractDocument.java:748)
	at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:717)
	at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:701)
	at org.eclipse.jface.text.AbstractDocument.fireDocumentChanged(AbstractDocument.java:775)
	at org.eclipse.jface.text.AbstractDocument.replace(AbstractDocument.java:1100)

Steps:

  • Open large file
  • Select all
  • Press Delete to empty the file

If you're patient enough, a subsequent Undo will do basically the same:

@sebthom
Copy link
Member

sebthom commented Jun 19, 2023

Because of #544 I am currently in the process of (probably) rewriting big parts of the model package. Once I got a handle on that either these issues are solved or I will have a second look at performance.

Anyways, you should already see considerable improvements in the performance using the snapshot build. can you confirm that?

@szarnekow
Copy link
Author

Yes, I can confirm that opening editors for bigger jsons is a lot better. Editing larger chunks of data is still slow, though. Instead of doing the big rewrite, do you mind implementing the suggestion in #543 (comment) ?

@sebthom
Copy link
Member

sebthom commented Jun 19, 2023

The changes you suggested are not necessary in that form in my rewrite. The UI thread will just need to add the information about the number of line changed to a queue. The insertion of ModelLine instances into the lines ArrayList (which can be expensive for large files), will then occur asynchronously in the tokenizer thread.

@sebthom
Copy link
Member

sebthom commented Jun 21, 2023

@szarnekow please give the latest snapshot from download.eclipse.org/tm4e/snapshots a try.

As suggested by you I changed the list modifying code to use List.subList() for deleting multiple lines at once. See https://github.com/eclipse/tm4e/blob/9b03a29dd214f5390aaf4f9fd05ed40d3a7b13ba/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/model/TMModel.java#L343

@szarnekow
Copy link
Author

@szarnekow please give the latest snapshot from download.eclipse.org/tm4e/snapshots a try.

I tried to, but the installation failed:

An error occurred while collecting items to be installed
session context was:(profile=<redacted>, phase=org.eclipse.equinox.internal.p2.engine.phases.Collect, operand=, action=).
Artifact not found: https://download.eclipse.org/tm4e/snapshots/plugins/org.eclipse.tm4e.ui_0.7.0.202306211001.jar.
https://download.eclipse.org/tm4e/snapshots/plugins/org.eclipse.tm4e.ui_0.7.0.202306211001.jar
Artifact not found: https://download.eclipse.org/tm4e/snapshots/features/org.eclipse.tm4e.feature_0.5.6.202306211001.jar.
https://download.eclipse.org/tm4e/snapshots/features/org.eclipse.tm4e.feature_0.5.6.202306211001.jar

@sebthom
Copy link
Member

sebthom commented Jun 21, 2023

Strange, well I bumped some versions and ran a rebuild. Can you try again please?

@szarnekow
Copy link
Author

Strange, well I bumped some versions and ran a rebuild. Can you try again please?

@sebthom I managed to install the new version, but it looks like it's quite broken. Please watch the attached screen recording. Two notable things:

  • functionally the highlighting is broken after edits
  • non-functionally the latency is quite bad. I was continuously typing and you can see the cursor / editing actions to be extremely laggy. Note how the cursors doesn't even blink most of the time
Screen.Recording.2023-06-24.at.15.27.30.mov

@sebthom
Copy link
Member

sebthom commented Jun 24, 2023

@szarnekow thanks for taking the time to try out the release. I unfortunately cannot reproduce the issue. JSON editing works just fine for me even for large files such as https://github.com/json-iterator/test-data/blob/master/large-file.json.

Two things:
a) can you provide me that json file you are experiencing the rendering issue?
b) can you verify that you have these plugin versions installed:
image

@mickaelistria
Copy link
Contributor

@sebthom the versions here show qualifier without an actual qualifier, which means they are not built versions and thus most likely not the artifacts that are available in the snapshots repo but your local working copy of those bundles.

@sebthom
Copy link
Member

sebthom commented Jun 26, 2023

Ok, this are the released vesions. Editing works fine for me.

image

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

No branches or pull requests

3 participants