Skip to content

Conversation

@jviide
Copy link
Collaborator

@jviide jviide commented Dec 18, 2018

This pull request modifies the cache key generation to use JSON.stringify. With this the cache key is guaranteed to be unique for every unique array of statics, as two different arrays should not have identical JSON serializations.

JSON.stringify is now stored to a module-scoped variable, to cut down the size. This seems to give a (surprising) performance boost in my environment. I suspect this is highly JavaScript engine specific. Benchmarks below.

Before change:
Creation: 36985/s, average: 0.027037810036229928ms
After change:
Creation: 45726/s, average: 0.02186895613191043ms

The compressed code size also decreased somewhat.

Before change:

Build "htm" to dist:
        625 B: htm.mjs.gz
        572 B: htm.mjs.br
        690 B: htm.umd.js.gz
        623 B: htm.umd.js.br

After change:

Build "htm" to dist:
        622 B: htm.mjs.gz
        568 B: htm.mjs.br
        686 B: htm.umd.js.gz
        618 B: htm.umd.js.br

Storing JSON.stringify in a variable but keeping the cache key generation intact (statics.length + statics) seems to yield the same performance, with a slightly larger gzipped code (624B instead of 622B). Uniqueness guarantee for cache keys is also lost.

@jviide jviide mentioned this pull request Dec 18, 2018
@developit
Copy link
Owner

Funky - I was just trying this out too! I didn't try storing a reference to stringify though, that's a really strange performance thing.

I set up a benchmark to check these: https://esbench.com/bench/5c18ff474cd7e6009ef61e65
From that it seems like JSON.stringify (+ cached) is quicker than reduce(), but both are slower than the inline loop:

screen shot 2018-12-18 at 9 33 53 am

@developit
Copy link
Owner

@jviide I added a second benchmark to measure cached template usage separately from creation:
e42f58d

@jviide
Copy link
Collaborator Author

jviide commented Dec 18, 2018

A small note: in the inline loop and reduce there needs to be a non-number character between statics[i].length and statics[i]. Otherwise there may be cache key ambiquity issues because the border of the length and the string becomes blurry. An example of this are the following templates (contrived, but illustrate the problem):

  • <a>${foo}9aaaaaaaaa${bar}</a>
  • <a>${foo}0${quux}aaaaaaaaa${bar}</a>

If the lengths are not separated by a some (non-number) character then both of them have the cache key 3<a>109aaaaaaaaa3</a>. Therefore I suggest changing statics[i].length + '' + statics[i] to something like statics[i].length + ',' + statics[i]. This would solve the ambiquity.

@developit
Copy link
Owner

developit commented Dec 18, 2018

heh yeah I noticed that! updated the benchmark too.

I replaced the map() test with a slight improvement on the loop one: instead of initializing key to an empty string, initializing it to a non-numeric character seems to trigger more optimal key lookups, at least in V8.

@jviide
Copy link
Collaborator Author

jviide commented Dec 18, 2018

Interesting. Is the updated benchmark accessible somewhere? The one linked above still seems to have the statics[i].length + '' + statics[i] (with the empty string between the statics[i].length and statics[i]).

@developit
Copy link
Owner

forgot to save - it's updated now

Prefix the key with a non-numeric character, as this seems to give a
speed-up on Chrome.
@jviide
Copy link
Collaborator Author

jviide commented Dec 18, 2018

Modified the cache key creation based on benchmarks. Now the key is a run-length encoding of statics. The key is also prefixed with a non-numeric character, which seems to give a boost on Chrome. Added tests.

Edit: It's a bit too late to change the commit message, so let's mention here that when I wrote "run-length encoding" I actually meant length-prefixed strings. RLE means something different.

@developit developit merged commit d517751 into developit:custom-jsx-parser Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants