Add new pref maxCodeHints

[marcelgerber]

For #9529.
I confirmed this is a performance boost for Code Hints.

[TomMalbran]

You could just do: this.maxResults = Math.min(maxResults || 1000, 1000); and remove the next 3 lines of code.

[marcelgerber]

I thought the same at first, but then thought it may confuse some.
Let's try it.

[redmunds]

Issue #9529 is about a performance lag which it assumes is caused by the length of the code hints list. Did you do any measurements to see what's the difference with and without your changes?

It could be that most of the time is with collecting the list (which is not addressed here) as opposed to building/displaying the list (which is addressed here). Might need to pass the maxResults to the provider in the getHints() API call to get the desired improvement.

[marcelgerber]

I haven't done exact measuring, but it felt faster using this branch. I used a workflow similar to the one mentioned in the issue linked above.

[marcelgerber]

So, I've just measured the time _updateHintList() takes (in a test environment with q library in the folder).
With maxCodeHints=1000, it took 255 and 281 ms; with maxCodeHints=50, it took 110 and 106 ms.
That's a noticeable difference.

I wonder though if we can improve CodeHintList.update as that seems to be the performance bottleneck.

[redmunds]

This maxCodeHints validation code should be moved to CodeHintList so all validation is done in 1 place.

[redmunds]

Done with review. Yes, this is noticeably faster. Just 1 comment.

[marcelgerber]

@redmunds Ready for final review.

[TomMalbran]

To improve CodeHintList.update we could use transforms instead of absolute positions and make use translatez(0) (eventually we should use the new will-change property) to make chrome use a new rendering layer for the code hints list and reduce the number of reflows.

[redmunds]

@TomMalbran We're not going to expand the focus of this pull request at this point, but I'm curious as to how you think using transforms could improve performance here. The code hint list is an absolutely positioned element so it should never cause a reflow of anything else, right?

[redmunds]

@marcelgerber I wasn't sure if the validation could would handle non-numeric cases (since the value comes from a json file), so I played around with this a bit.

The good news is that all non-numeric types seems to be correctly ignored.

The bad news is that floating point numbers seem to cause a problem. When I set value to 5.5, then the number of items is rounded up to 6, which is reasonable. But, when you use up/down arrows to navigate list and go past end (which should wrap around), then the selection jumps out of the list and into the doc!

Need to add some validation code to force value to be a whole number.

[TomMalbran]

I wast just commenting on something @marcelgerber said and I agree that it should be another PR.

It is actually painting a big rectangle at the bottom-left corner of the editor, that it shouldn't. Using translate should also makes it faster to calculate. Maybe the removing the box shadow could help too.

[marcelgerber]

@redmunds Integer validation is in.

[redmunds]

Need to add a @param for maxResults. I can see that it's not part of your PR, but can you also add one for insertHintOnTab too?

[marcelgerber]

Done.

[redmunds]

@TomMalbran

It is actually painting a big rectangle at the bottom-left corner of the editor, that it shouldn't.

I'm not seeing that. Open an issue if you have a recipe for it.

Maybe the removing the box shadow could help too.

I don't think @larz0 would go for that :)

[redmunds]

Looks good. Merging.

[redmunds]

@marcelgerber Can you add new pref to this doc? https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#preferences

[marcelgerber]

@redmunds Done.

@TomMalbran I did some performance measurements and AFAICT, the real bottleneck is not the rendering, but the creation.
The jQuery loop takes much time, but I'm not sure if we can change something there.
Positioning isn't fast either.

[TomMalbran]

@marcelgerber I was able to remove that loop and create the jQuery objects and pass them to Mustache when rendering the list the first time. But since I still need to create the html from jQuery I am not sure if that makes it any faster. It might get faster if we could give a formatting function to the CodeHintsList and it could use it to format just the items that were required. Maybe using React here could help too, which could allow use to use virtual scrolling.

[TomMalbran]

@redmunds @marcelgerber Changing the position of an element trigger layout, paint and composition, while using transforms only triggers composition (http://csstriggers.com/) which is why it is faster. I heard that Chrome is trying to fix this, but until then, using transforms should be faster.

[peterflynn]

I find the dense logic here a little hard to follow. It seems more clear if we did something like:

this.maxResults = ValidationUtils.isIntegerInRange(maxResults, 1, 1000) ? maxResults : 1000;

or

if (ValidationUtils.isInteger(maxResults) && maxResults > 0) {
    this.maxResults = Math.min(maxResults, 1000);
} else {
    this.maxResults = 1000;
}

[peterflynn]

It seems odd to me that the pref defaults to 50 but the API defaults to 1000. Any reason for the difference?

We could still cap the arg at 1000 but have it default to 50 if unspecified -- the 2nd of my code suggestions above would leave the code still pretty easily understandable if we did that.

[marcelgerber]

Yeah, seems reasonable - feel free to open a PR.
But you should first check maxResults > 0.