Finalizes incremental settings

[NikolasKomonen]

Fixes #133

[NikolasKomonen]

@fbricon updated

[NikolasKomonen]

Current issues, a large file will break incremental parsing. If many characters are typed quickly the document on the server side ends up misrepresenting the actual document.

Difficult to debug this.

[angelozerr]

@NikolasKomonen please see #369 which is a PR just with an idea which could fix perhaps bug with large file. Not sure it works well, but this PR is just to open discussion

[angelozerr]

Nick i seen that you integrated my work . could you give me feedback please if it fix your problem which is difficult to debug. Thanks

[NikolasKomonen]

@angelozerr I've been testing it on a file with 140,000 lines and it seems to be working properly. There are no more synchronization issues.

The file, nasa.xml, in src/test/resources/xml will crash the extension. I'm not sure if that is on VSCode or lsp4xml.

Otherwise, from a regular use case it looks good to me for now.

[angelozerr]

Thanks for your feedback.

Could you explain more the use cases which crashes vscode please. Imho we need to improve performance and or memory problem because i see more and more issues about that

[NikolasKomonen]

For the nasa.xml file, while waiting for initialization of the server to finish VS and return to the client side I get an error from VSCode "Extension host terminated unexpectedly"

(I've set the server memory to 4GB by the way)

Error:

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [/usr/share/code/libnode.so]
 2: 0x7fe6994ea867 [/usr/share/code/libnode.so]
 3: 0x7fe698c2ae5e [/usr/share/code/libnode.so]
 4: 0x7fe698c2ade9 [/usr/share/code/libnode.so]
 5: v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [/usr/share/code/libnode.so]
 6: 0x7fe699123670 [/usr/share/code/libnode.so]
 7: 0x22976a842dd

From other issues I've looked at it could be to do with node and
throwing the flag--max_old_space_size=8192 in the node build cli command could fix it.
I'm just not sure where that would be. @fbricon any ideas?

[fbricon]

Looks like vscode doesn't have enough memory to open the document, if you can reproduce that after disabling all the vscode extensions, then you should open a ticket in https://github.com/Microsoft/vscode/issues.

[NikolasKomonen]

Per @fbricon 's recommendation, I tested the nasa.xml file with the HTML language server and that had the same error with the HTMLLS crashing too.

[angelozerr]

Please remove buffer field because the TextDocument stores 2 xml content in this case (buffer and text field). It's not good for memory (2 XML content stored). So the idea is to create StringBuilder instance from the current text in the update method to benefit with StringBuilder#replace method. and synchronized to a lock field instance (not to the buffer field which must be removed)

private final Object lock = new Object();

...
public void setIncremental(boolean incremental) {
		this.incremental = incremental;
	}

	public boolean isIncremental() {
		return incremental;
	}
...
public void update(List<TextDocumentContentChangeEvent> changes) {
		if (isIncremental()) {
			try {
				StringBuilder buffer = new StringBuilder(getText());
				synchronized (lock) {
...

An another change is to set lineTracker as final because the call of getLineTracker can be created several time when there are several threads which are called it in the same time, we could use synchronized but we need not that because lineTracker.set exists as methods.

private final ListLineTracker lineTracker;

	public TextDocument(String text, String uri) {
		this.lineTracker = new ListLineTracker();
		super.setUri(uri);
		setText(text);
	}
       ...

	@Override
	public void setText(String text) {
		super.setText(text);
		lineTracker.set(text);
	}

Here the final TextDocument with those 2 changes:

/**
 *  Copyright (c) 2018 Angelo ZERR.
 *  All rights reserved. This program and the accompanying materials
 *  are made available under the terms of the Eclipse Public License v2.0
 *  which accompanies this distribution, and is available at
 *  http://www.eclipse.org/legal/epl-v20.html
 *
 *  Contributors:
 *  Angelo Zerr <[email protected]> - initial API and implementation
 */
package org.eclipse.lsp4xml.commons;

import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.eclipse.lsp4j.Position;
import org.eclipse.lsp4j.Range;
import org.eclipse.lsp4j.TextDocumentContentChangeEvent;
import org.eclipse.lsp4j.TextDocumentItem;

/**
 * Text document extends LSP4j {@link TextDocumentItem} to provide methods to
 * retrieve position.
 *
 */
public class TextDocument extends TextDocumentItem {

	private static String DEFAULT_DELIMTER = System.lineSeparator();

	private final ListLineTracker lineTracker;

	private boolean incremental;

	private final Object lock = new Object();

	public TextDocument(TextDocumentItem document) {
		this(document.getText(), document.getUri());
		super.setVersion(document.getVersion());
		super.setLanguageId(document.getLanguageId());
	}

	public TextDocument(String text, String uri) {
		this.lineTracker = new ListLineTracker();
		super.setUri(uri);
		setText(text);
	}

	public void setIncremental(boolean incremental) {
		this.incremental = incremental;
	}

	public boolean isIncremental() {
		return incremental;
	}

	@Override
	public void setText(String text) {
		super.setText(text);
		lineTracker.set(text);
	}

	public Position positionAt(int position) throws BadLocationException {
		ListLineTracker lineTracker = getLineTracker();
		return lineTracker.getPositionAt(position);
	}

	public int offsetAt(Position position) throws BadLocationException {
		ListLineTracker lineTracker = getLineTracker();
		return lineTracker.getOffsetAt(position);
	}

	public String lineText(int lineNumber) throws BadLocationException {
		ListLineTracker lineTracker = getLineTracker();
		Line line = lineTracker.getLineInformation(lineNumber);
		String text = super.getText();
		return text.substring(line.offset, line.offset + line.length);
	}

	public String lineDelimiter(int lineNumber) throws BadLocationException {
		ListLineTracker lineTracker = getLineTracker();
		String lineDelimiter = lineTracker.getLineDelimiter(lineNumber);
		if (lineDelimiter == null) {
			if (lineTracker.getNumberOfLines() > 0) {
				lineDelimiter = lineTracker.getLineInformation(0).delimiter;
			}
		}
		if (lineDelimiter == null) {
			lineDelimiter = DEFAULT_DELIMTER;
		}
		return lineDelimiter;
	}

	public Range getWordRangeAt(int textOffset, Pattern wordDefinition) {
		try {
			Position pos = positionAt(textOffset);
			ListLineTracker lineTracker = getLineTracker();
			Line line = lineTracker.getLineInformation(pos.getLine());
			String text = super.getText();
			String lineText = text.substring(line.offset, textOffset);
			int position = lineText.length();
			Matcher m = wordDefinition.matcher(lineText);
			int currentPosition = 0;
			while (currentPosition != position) {
				if (m.find()) {
					currentPosition = m.end();
					if (currentPosition == position) {
						return new Range(new Position(pos.getLine(), m.start()), pos);
					}
				} else {
					currentPosition++;
				}
				m.region(currentPosition, position);
			}
			return new Range(pos, pos);
		} catch (BadLocationException e) {
			return null;
		}
	}

	private ListLineTracker getLineTracker() {
		return lineTracker;
	}

	/**
	 * Update text of the document by using the changes and according the
	 * incremental support.
	 * 
	 * @param changes the text document changes.
	 */
	public void update(List<TextDocumentContentChangeEvent> changes) {
		if (changes.size() < 1) {
			// no changes, ignore it.
			return;
		}
		if (isIncremental()) {
			try {
				synchronized (lock) {
					// Initialize buffer and line tracker from the current text document
					String initialText = getText();
					StringBuilder buffer = new StringBuilder(initialText);
					ListLineTracker lt = new ListLineTracker();
					lt.set(initialText);
					// Loop for each changes and update the buffer
					for (int i = 0; i < changes.size(); i++) {
						if (i > 0) {
							lt.set(buffer.toString());
						}
						TextDocumentContentChangeEvent changeEvent = changes.get(i);
						Range range = changeEvent.getRange();
						int length = 0;

						if (range != null) {
							length = changeEvent.getRangeLength().intValue();
						} else {
							// range is optional and if not given, the whole file content is replaced
							length = buffer.length();
							range = new Range(lt.getPositionAt(0), lt.getPositionAt(length));
						}
						String text = changeEvent.getText();
						int startOffset = lt.getOffsetAt(range.getStart());
						buffer.replace(startOffset, startOffset + length, text);
					}
					// Update the new text content from the updated buffer
					setText(buffer.toString());
				}
			} catch (BadLocationException e) {
				// Should never occurs.
			}
		} else {
			// like vscode does, get the last changes
			// see
			// https://github.com/Microsoft/vscode-languageserver-node/blob/master/server/src/main.ts
			TextDocumentContentChangeEvent last = changes.size() > 0 ? changes.get(changes.size() - 1) : null;
			if (last != null) {
				setText(last.getText());
			}
		}
	}
}

[angelozerr]

@NikolasKomonen are you sure that you have done the update of TextDocument like I suggest below?

[NikolasKomonen]

@angelozerr I'm getting offset issues with the changes. Did you try running tests with your implementation?

[angelozerr]

Yes it should work. What is your test (and traces) which failed ?

[NikolasKomonen]

My failures are that it "Couldnt get the position at Offset", and BadLocation errors.

I pushed the changes to this PR, I'm sorry if I missed anything from your changes.