search

SHsuperCM / CITResewn

Fabric implementation of mcpatcher's cit
MIT License
154 stars 76 forks source link

Improve performance of NBT String parsing #320

Closed wahfl2 closed 4 months ago

wahfl2 commented 10 months ago

As my last PR was closed after a review instead of giving me time to implement the requested changes, I'm opening another one. This still gives roughly the same performance boost as #311; see that for screenshots.

This PR implements a much less intrusive optimization, adding only 7 characters to 1 line of code. It should not change the behavior of CITResewn in any way, shape, or form.

The second commit might be slightly more "controversial" since I didn't do extensive testing to see if it would/could throw any other exception. The owner may choose not to include this commit.

wchristian commented 9 months ago

Given that SHsuperCM seems to be on a hiatus, i've created a temporary patch fork with a release file under https://github.com/wchristian/CITResewnJSONLagPatch which will be available on Modrinth and Curseforge once moderation has approved them.

Thanks for the research and the patch, wahfl2. :)

wchristian commented 9 months ago

And it's available on modrinth and curseforge now:

https://modrinth.com/mod/cit-resewn-jsonlagpatch https://legacy.curseforge.com/minecraft/mc-mods/cit-resewn-jsonlagpatch

wchristian commented 9 months ago

i just pushed out v1.1.3.1 of cit-resewn-jsonlagpatch to modrinth and curseforge with #343, which contains an additional performance improvement originally made by @nea89o, with slight adjustments for earlier feedback from SHsuperCM in #315

https://modrinth.com/mod/cit-resewn-jsonlagpatch/versions https://legacy.curseforge.com/minecraft/mc-mods/cit-resewn-jsonlagpatch/files

SHsuperCM commented 4 months ago

So a few things,

  1. The characters added or removed is not a problem.
  2. Both fromJson and fromLenientJson perform roughly the same, the difference between them is negligible and it is to do with the path they take to get to gson's JsonReader.
  3. Lenient does not mean it'll be lenient on exceptions, lenient is passed directly into Gson's parser and modifies what the parser allows as values, which both is irrelevant to this issue and could potentially break expected behavior. Lenient still throws exceptions.
  4. Gson actually uses lenient parsing regardless by default, when passing through JsonParser.parseString (which is what fromJson uses) it still goes through parseReader which forces lenient reading. The only difference being that when using parseString it wraps certain exceptions in JsonExceptions.
  5. Barring lenient still throwing exceptions, all the 2nd commit removing the trycatch does is propagate upwards to be caught elsewhere. The expected behavior from test is to return true or false, the caller should not be concerned if there is a "malformed" json in there or not, tests yield true or false.
  6. While I closed the last PR instead of requesting changes for a very good reason. This does not stop you from requesting the PR to be reopened.

I am closing this PR and again, I have good reason to do so.