JSON rewrite

[miparnisari]

[miparnisari]

Fixes #819

[ffes]

I hope one of the members of this project finds the time to review this

[pyrmont]

@miparnisari I'm sorry this has been outstanding for so long :( I'm working my way through the backlog and hope to get to this soon!

[jneen]

For the json lexer in particular, performance on large inputs is important. Would you mind testing with larger data? I believe if you search the issues for "json" you will find a few issues that prompted us to make the lexer extremely simple.

Also @pyrmont for avoiding future performance issues I wonder if it might be useful to have some kind of max_length option for the lex - where if the input is longer than the max length, it will give up and highlight the rest as Text. For use cases like Gitlab where they can't control the code this sort of thing could be very helpful.

[pyrmont]

I had a look through the issues and while I think I found the ones that @jneen is referring to (see #41, #114, #244), they don't actually include an example of the JSON being tested.

I made one using Mockaroo and have put it in a gist. It's not as large as the 200KB one referenced in #41 but you could always just double it if a larger file is desired.

I'm personally curious what the speed difference is with newer versions of Ruby and will try to give it a spin a bit later.

[pyrmont]

@jneen Yeah, a max_length option sounds like an idea worth serious consideration. Will have a bit of a think about this, too.

[jneen]

Now that I look closely, I think #525 is relevant (the actual issue there was with the JSON lexer)

[jneen]

the offending case was a string that contained a large number of backslashes: https://github.com/jneen/rouge/files/410135/bad_note.txt

[miparnisari]

I'm personally curious what the speed difference is with newer versions of Ruby and will try to give it a spin a bit later.

Can you please edit the README to include instructions on how to measure performance of two lexers? Or even in the PR template.

[pyrmont]

@miparnisari wrote:

Can you please edit the README to include instructions on how to measure performance of two lexers? Or even in the PR template.

I don't think generally we'd expect submissions to include speedtests; this felt like a special case because of: (1) the prevalence of JSON; and (2) the fact JSON files might realistically be quite large.

That said, providing some 'official' support for benchmarking doesn't sound like a bad idea. I was planning on using the Shootout library mentioned in #114 and will try to see how simple it can be made to test different versions of Rouge (it's really designed to benchmark Rouge against different syntax highlighting libraries which isn't quite what I'm concerned with). To compare versions of Rouge, I was anticipating switching between git branches and then running the tests.

Oh, and as a bonus, I realised the 200KB JSON file is in there.

[miparnisari]

a max_length option sounds like an idea worth serious consideration. Will have a bit of a think about this, too.

Agree. I know my company has this logic in place - if the file is too big, it bails out and renders it as plain text.

I don't think generally we'd expect submissions to include speedtests;

Agree. It could be an optional input of the PR.

For the json lexer in particular, performance on large inputs is important. Would you mind testing with larger data?

I tried the 200 kb file. When I use rackup, both this and the existing lexer take around 1.5 sec. (I hope this is correct, I looked at the last number in the request log)

[pyrmont]

I got Shootout working and ran it against both this PR and the current master.

                      master               PR #1029
JSON (217 kB)
=> terminal           1209 kB/s            1039 kB/s
=> html               1454 kB/s            1203 kB/s

There's definitely a slow down: terminal is 14% slower and HTML is 17% slower. That's not great but I'm inclined to think this is acceptable given the fact this has been requested multiple times and does seem like an overall usability win.

What do you think @jneen?

[jneen]

The code for rouge.jneen.net (in this same org) contains a class that can be used to run different versions of rouge in the same runtime, which might be helpful.

[pyrmont]

@miparnisari Thanks for keeping this up to date. I plan to have a look tonight at the case @jneen linked to earlier. I never did actually check that.

[pyrmont]

@miparnisari Sorry, I didn't mean to submit two commits in quick succession but as I was writing a message to explain why the code didn't support highlighting what I'm calling 'bare' key-value pairs for performance reasons, I realised there was a way to do it that didn't involve excess backtracking.

In summary, these commits makes several changes to the proposed lexer:

• First, they change the tokens used in JavaScript objects. It was proposed that keys be tokenised as Str::Double and that string values be tokenised as Name::Tag. The token Name::Label seems a more appropriate token for keys while Str::Double should be used for string values.

• Second, they remove the tokenisation of escape sequences inside object keys. While this tokenisation was correct insofar as a JSON parser would parse the string, it produces an odd result where Name::Label token can be interrupted by Str::Escape tokens.

• Third, they add support for 'bare' key-value pairs, that is text of the form <string>: <value>. This preserves backwards compatibility with how the JSON lexer previously worked. These 'bare' key-value pairs are tokenised roughly the same way as they would be within an object. The difference is that escaped characters are not tokenised differently to other characters in a string. This seems an acceptable trade-off to me given that bare strings are not correct JSON in the first place and so how they should be tokenised outside of an object is not really defined.

Hopefully that all makes sense. I'll try to add some more examples tonight to the visual sample to demonstrate how this works.

[pyrmont]

@miparnisari My changes aren't working for subsequent bare key-value pairs. Need to have more of a think about this one.

[pyrmont]

@miparnisari Sorry for overwriting your repository. It was the only way I could see to get the branch caught up to the latest version. I needed to do that because the HOCON lexer (which did not exist at the time you created this branch) subclasses the JSON lexer. Since we're pretty radically changing how it works, that broke that lexer.

It's all fixed now and looks to be making all those keys look nice and pretty :)