-
Notifications
You must be signed in to change notification settings - Fork 9
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
Support validation on didSave and/or add debounce for didChange #613
Comments
Hi @deiteris ! We do have some mechanisms to try and improve performance and usability:
The issue is probably the same in both JVM and JS, the main difference being that JVM manages to better use the PC resources (more specifically, it provides multi-threaded processing). If what you want is to turn off automatic validations and just validate on demand, I suggest you look at If there is a specific optimization you have in mind (please be aware that cancellation is not an option at this time), please let us know. Performance/consistency is always a priority for us and any idea to improve this is always welcomed with open arms. |
Hi @llibarona!
I am aware of this custom request, but if I would like to use this approach, how can I disable automatic validation?
Hmm, from my observations, I see that actually each change done (probably there's some time window since it's not character by character) to the file is being parsed. While with JVM I see previous entries being processed fast in parallel (and this actually may put a lot of strain even on powerful PC too), in JS I see requests stacking and being processed one by one with single thread (worker threads could probably put performance to the same level as in JVM).
As I mentioned in my initial post, debouncing class TextDocumentManager(...) extends AlsTextDocumentSyncConsumer {
...
private var prevRequestTime: Long = System.currentTimeMillis
override def didChange(params: DidChangeTextDocumentParams): Unit = {
...
val curRequestTime = System.currentTimeMillis
// Threshold in millis, could be configurable.
val requestThreshold = 1000
if ((curRequestTime - this.prevRequestTime) > requestThreshold) {
documentWasChanged(ChangedDocument(uri.toAmfUri, version, text, None))
}
this.prevRequestTime = curRequestTime
}
...
} The other way around would probably be running an AMF instance for each file on a separate thread (using Java thread and JS worker thread), let some number of threads run and finish execution so we have some parsed document data available ASAP (like it seems to be done right now), and stopping thread execution when cancellation request was received (so we don't rely on AMF implementation), but this looks like more work and investigation to do. Sorry if it doesn't really make sense or was already researched, since I have little idea how the server is currently designed, what possible limitations can be when maintaining both versions and can mostly judge from the way I see it works :) |
Hi @deiteris , great research As of 4.1.0 ( Now in In ALS we have multiple managers for specific tasks (requests/notifications) Inside the
1 and 2 will trigger the project parse, which means it will remove from the queue any notification referencing the main file or tree (as it is already processed) This process (which should only be occurring one at a time per When the process is done, the new result is kept in the Any other request should not trigger directly a parse/resolution/validation, instead it just consumes the last one from the (sorry for the long expo) Regarding multi threaded performance in JS, unfortunately ScalaJS does not support this as far as I'm aware of. There is always the possibility to implement this low-level obviously, but it has it's risks and maintainability issues. Regarding the multithreaded performance in JVM, are you able to confirm if there are in fact multiple process calls at the same time in Through the telemetry notifications this should be quite visible, as each parse/resolution/diagnostic should send a telemetry notification on start and finish, with a UUID (so each start should match a finish with no other start in between)
Regarding the debouncing mechanisms, it is a strong candidate for improvement, we talked about it in the past and never came up with a strategy we felt confortable with, and also never encountered that many strong cases in favor of doing so, but I will refloat the talks about it I'm sorry for the verbose explanation, and hope it was somewhat clear. |
Great explanation :) It's a bit hard to wrap my head around since it's a really complex subject while I am also getting to understand Scala a bit. But now I see the limitations, and things aren't as bright as I was imagining in the beginning.
I've just re-checked, and no, there are no other But now I did direct comparison (with version 4.1.0) and figured, that JS variant is almost 10x times slower than JVM when it comes to parsing alone, and 6-7x times slower in other scenarios like document linking, folding in the same RAML API definition. Unfortunately, I cannot share the API definition without a permission, but I can say that it has around 130 endpoints and each one has a JSON schema (there are also some common schemas linked as type definitions) + examples. Here're few telemetry entries: JVM
JS
So, this actually requires more thorough investigation whether things can be improved for JS variant specifically. I see good amount of work done on optimization, and not sure I can actually give a better idea now. The best idea I currently have, is adding an option to do all processing on save (or on demand). Yes, we'll loose some fancy functionality if we choose to enable it, but this will help to avoid stacking lots of requests when the API is too large or we don't have fast enough PC to do the processing in real-time, that might take an eternity in our case :) Debouncing now looks not as good idea, as I was thinking, since your current code already implements better handling. Making an option with
But nevertheless, even with relatively worse performance, JS variant is a great alternative for smaller API definitions since it doesn't require any external dependencies to run the extension, performance penalty is not that critical for them and final size is much smaller (55mb for fat jar and 7.5mb for bundled js). |
That data you provide seems a bit concerning for us. edit: in case this was taken while editing the API, it could be explained by us waiting until the last change is parsed in order to respond (in that case, to the total time of the request we would need to subtract the sum of all parsing times in between) |
I noticed that I specified wrong main API file. Here's a bit more telemetry with JS variant starting from one of
Yes, the part I've shown in my previous post was taken while I was editing. In that case only small part was cut since it's hard to navigate in telemetry log, as it becomes huge even after few edits, and filter out the data I'm probably not allowed to show. I provided a bit more telemetry above that may give you a better insight.
Not sure what you mean. If by Project you mean whether I have main API file, then yes. But what do you mean by Dependencies?
According to AMF Graph that I generated with ALS, search by word Type definition (f.e., {
"type": "string",
// Some type constraints here
} Usage: {
"$schema": "http://json-schema.org/draft-04/schema#",
"definitions": {
"common_type": {"$ref": "common_type.json"},
},
"type": "object",
"properties": {
"prop": {
"$ref": "#/definitions/common_type"
},
},
...
} |
This should not affect our times that much. Looking at the telemetry logs, I see this:
It would look like the requests are stacking really bad, I created an internal ticket (https://www.mulesoft.org/jira/browse/ALS-1702) to look to improve this (ideally implementing a Thanks for the provided data! |
One thing I've noticed, and probably why this issue is especially noticeable in the JS variant, is that generated client never utilizes To confirm this, the search for |
Yes, we dabbled with this issue a few times, and found no out of the box way to enable true multithreading in the js client of scalaJS |
That's not exactly what I meant. Just to make sure we're on the same page, I'm saying that that node.js environment allows multithreaded processing for |
I am not sure I understand I will try and make a small POC to check if we are in the same page and show you so you can tell me if it is what you mean |
I gave it a shot, but cannot make the JS version to run in parallel For what I read (correct me if I'm wrong), (based on https://stackoverflow.com/a/46086037) |
You're on the right track but something with Scala.js code might be not right. Here are few notes:
Unfortunately, I'm unable to build from this branch to analyze transpiled code or see the test, so my current observations are based on the Scala source. Here're the errors I have:
|
sorry about that! |
sorry for the long wait! I rebased the branch and it should now be compiling (although it is only illustrative as the POC did not work as intended) |
Sorry for long response. I've looked into compiled code, and found that it doesn't use promises either. I was looking into Scala.js and the only thing I was able to figure out is that tasks could be enqueued using Due to my limited knowledge I'm out of suggestions for now and don't want to bother with speculations (and we diverged from the original issue anyway). Hope at least something of this was useful and may help in future! |
No problem! |
Your issue may already be reported! Please search on Github issues before creating one.
I'm submitting a ...
What is the current behavior?
Currently, the language server doesn't seem to handle
didSave
event (https://github.com/aml-org/als/blob/develop/als-server/jvm/src/main/scala/org/mulesoft/als/server/lsp4j/TextDocumentServiceImpl.scala#L88), anddidChange
event works in real-time.Either of the solutions would probably significantly improve the handling of large API definitions (and when working on slower PCs), since their validation may take some time. Providing validation with just
didChange
event in real-time in this case is suboptimal since the requests aren't executed on time and being enqueued and executed one by one (again, maybe this is how it works in JS specifically, with JVM I don't notice requests stacking) without any possibility to cancel them.Please tell us about your environment:
Other information (e.g. detailed explanation, stack traces, related issues, workarounds, links for us to have context, eg. StackOverflow, Gitter, etc)
The text was updated successfully, but these errors were encountered: