Oracle: Optimize json parsing #3480
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note that I might move to GraphQL to only serve what you request |
|
Isn't it faster to use regular expressions instead of indexof? |
|
Parsing time is considerably faster now, which is awesome. 👍 Could you still look into implementing a progress bar for that part, please? @ctrlaltca |
|
regular expressions are several orders of magnitude slower :/ |
|
Have you looked at using qjson ( http://qjson.sourceforge.net/ ) instead? We don't have to maintain a json parser ourselves. |
Looks like a good candidate, performance is comparable with the current implementation. |
Daenyth
approved these changes
Dec 26, 2018
tooomm
changed the title
ebbit1q
added a commit
to ebbit1q/Cockatrice
that referenced
this pull request
Jan 10, 2022
the long type has different sizes across operating systems and should not be used in the timeline an overflow could occur if the width in pixels multiplied by the total amount of milliseconds in the replay is larger than 2^31 which is easy enough considering with only 500 pixels width you'll reach this number with only 1.2 hours of replay (about 4 million millis), note that this would be windows exclusive as *nix uses 64 bits qt-json's own repo suggests using qt5's implementation instead, testing revealed this is quite a bit faster, contrary to Cockatrice#3480 servatrice uses the qthread usleep function which used to be protected but is now public cockatrice is not compatible with qt4 and hasn't been for a while
ebbit1q
added a commit
to ebbit1q/Cockatrice
that referenced
this pull request
Jan 10, 2022
the long type has different sizes across operating systems and should not be used in the timeline an overflow could occur if the width in pixels multiplied by the total amount of milliseconds in the replay is larger than 2^31 which is easy enough considering with only 500 pixels width you'll reach this number with only 1.2 hours of replay (about 4 million millis), note that this would be windows exclusive as *nix uses 64 bits qt-json's own repo suggests using qt5's implementation instead, testing revealed this is quite a bit faster, contrary to Cockatrice#3480 servatrice uses the qthread usleep function which used to be protected but is now public cockatrice is not compatible with qt4 and hasn't been for a while
ebbit1q
added a commit
to ebbit1q/Cockatrice
that referenced
this pull request
Jan 10, 2022
qt-json's own repo suggests using qt5's implementation instead, testing revealed this is quite a bit faster, contrary to Cockatrice#3480 after further investigation it became clear that this only works on the qt version 5.15 we are not ready to end support for older versions of qt just yet so maybe we can conditionally use the faster version instead?
ebbit1q
added a commit
to ebbit1q/Cockatrice
that referenced
this pull request
Jan 10, 2022
the long type has different sizes across operating systems and should not be used in the timeline an overflow could occur if the width in pixels multiplied by the total amount of milliseconds in the replay is larger than 2^31 which is easy enough considering with only 500 pixels width you'll reach this number with only 1.2 hours of replay (about 4 million millis), note that this would be windows exclusive as *nix uses 64 bits ~~qt-json's own repo suggests using qt5's implementation instead, testing revealed this is quite a bit faster, contrary to Cockatrice#3480~~ testing proved this to not be compatible with older qt versions servatrice uses the qthread usleep function which used to be protected but is now public cockatrice is not compatible with qt4 and hasn't been for a while
ZeldaZach
pushed a commit
that referenced
this pull request
Jan 16, 2022
the long type has different sizes across operating systems and should not be used in the timeline an overflow could occur if the width in pixels multiplied by the total amount of milliseconds in the replay is larger than 2^31 which is easy enough considering with only 500 pixels width you'll reach this number with only 1.2 hours of replay (about 4 million millis), note that this would be windows exclusive as *nix uses 64 bits ~~qt-json's own repo suggests using qt5's implementation instead, testing revealed this is quite a bit faster, contrary to #3480~~ testing proved this to not be compatible with older qt versions servatrice uses the qthread usleep function which used to be protected but is now public cockatrice is not compatible with qt4 and hasn't been for a while
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Ticket(s)
Short roundup of the initial problem
Since the move from Mtgson2 to Mtgjson4, the json file size increased a lot. This resulted in increased parsing time in oracle, eg. on my mac from 5-10 second to over a minute. We are not even reporting a progress indicator to users.
Back in the Qt4 days, Qt had no native support for Json, so Cockatrice is embedding a 3rd party library called qt-json: https://github.com/qt-json/qt-json
Nowadays Qt5 has a native solution to parse Json, but it's so optimized for speed on small files that it doesn't handle big json files at all. I've tried to use it, but it end up with a
too large documenterror.What will change with this Pull Request?
While investigating the speed of the json library, I've found out that a couple optimizations could speed up the parsing by about 3 times. The original patch was found here: qt-json/qt-json#20
On my mac, the parsing time is reduced from 63 to 21 seconds.
I'm keeping this PR open a couple of days to investigate other possible optimizations.