Make JSON::Parser initialization faster #512

luke-gru · 2023-01-29T14:40:34Z

Remove calls to rb_funcall, don't go back in to interpreter if not necessary. Also remove unnecessary calls to rb_respond_to(val) when val is nil. Speedup of JSON.parse when parsing small amount of data in a tight loop is up to 40%.

luke-gru · 2023-01-29T14:45:33Z

Here's the benchmark code:
https://gist.github.com/luke-gru/adf7468fbc0a3807b5f9a875fcfce116

On my machine:

Rehearsal ----------------------------------------------

json parse (old)  4.140547   0.001651   4.142198 (  4.262071)

------------------------------------- total: 4.142198sec



                 user     system      total        real

json parse (old) 4.099210   0.011923   4.111133 (  4.246992)

Rehearsal ----------------------------------------------

json parse (new)   3.163645   0.003476   3.167121 (  3.290742)

------------------------------------- total: 3.167121sec



                 user     system      total        real

json parse (new)  3.121472   0.004022   3.125494 (  3.198764)

mensfeld · 2023-01-29T14:55:04Z

Speedup of JSON.parse when parsing small amount of data in a tight loop is up to 40%.

😮

hsbt · 2023-01-30T01:10:08Z

@luke-gru Can you keep the current indent style? We can view only code changes with https://github.com/flori/json/pull/512/files?w=1. But style changes may causes code-conflict when I backport this to ruby/ruby.

luke-gru · 2023-01-30T19:37:40Z

HI @hsbt, for the JSON init function, it's a bit subtle but I had to remove the check if (!NIL_P(opts)) and move it below where I could assure that opts was a Hash, so I could use RHASH_SIZE() on it. So for that function it's not just a style change. For parser.c, I think I used a different version of ragel and all the whitespace changed. I could fix this, do you want me to look into it?

EDIT: my ragel is Ragel State Machine Compiler version 6.10 March 2017

hsbt · 2023-01-30T21:53:33Z

So for that function it's not just a style change.

I know that. I say parser.rl and parser.h. I hope you make your change is only https://github.com/flori/json/pull/512/files?w=1, not https://github.com/flori/json/pull/512/files.

luke-gru · 2023-01-30T22:58:49Z

Understood, sorry for the confusion. Just updated it.

Remove calls to rb_funcall, don't go back in to interpreter if not necessary. Also remove unnecessary calls to rb_respond_to(val) when val is nil. Speedup of JSON.parse when parsing small amount of data in a tight loop is up to 40%.

nobu · 2023-02-07T03:35:35Z

ext/json/ext/parser/parser.rl

+        if (json->decimal_class != Qnil) {
        if (rb_respond_to(json->decimal_class, i_try_convert)) {


Seems indents broken.

If you want to keep the indent of the following block,

Suggested change

if (json->decimal_class != Qnil) {

if (rb_respond_to(json->decimal_class, i_try_convert)) {

if (NIL_P(json->decimal_class)) {

/* use the default conversion */

} else if (rb_respond_to(json->decimal_class, i_try_convert)) {

ext/json/ext/parser/parser.rl

nobu · 2023-02-07T05:59:21Z

ext/json/ext/parser/parser.h

-#define option_given_p(opts, key) RTEST(rb_funcall(opts, i_key_p, 1, key))
+extern VALUE rb_hash_has_key(VALUE hash, VALUE key);
+
+#define hash_option_given_p(opts, key) (RTEST(rb_hash_has_key(opts, key)))


Does this name need to be changed?

Is there a problem with using a non public API?

When I tried before, I created my own function with the same effect. #481

Hi @Watson1978 I wasn't aware of your PR, I should have checked the other PRs more thoroughly before coding up my solution. Yours is better IMO with using only public API, and also making changes to JSON.generate. The only thing that my PR has that's different in the initialize function is that it checks if the hash is empty before going through all the option checks, which yours could add if you want. I also think your other PR (#454) could be merged with the changes in #481.

I'm fine with closing my PR now if yours gets a second look. However if you're busy I'll just take some ideas from your PR and add to mine.

Is there a problem with using a non public API?

Yes, that is not respecting the API and will likely break. Also it would likely not work e.g. on truffleruby needlessly.

Okay, fixed

hsbt · 2023-02-07T06:47:18Z

I accept this proposal. But I'm not familiar with C internals. I requested additional review for @nobu

mensfeld · 2023-03-21T09:46:31Z

@luke-gru @hsbt @nobu any ETA on this being released?

luke-gru · 2023-03-21T13:32:06Z

Yeah, I'll try to get this updated some time today. Thanks for the reminder 😄

luke-gru · 2023-04-03T16:32:24Z

I finally got around to working on this. I added a 2nd commit so you can see the changes from the first. When approved, I will squash them.

mensfeld · 2024-05-08T09:57:03Z

@luke-gru @hsbt @nobu any ETA on this being released? :D (it's more than a year from the last time I asked so hope you don't mind)

Extracted from: ruby#512 Use `rb_hash_lookup2` to check for hash key existence instead of going through `rb_funcall`. Co-Authored-By: lukeg <luke.gru@gmail.com>

byroot · 2024-10-17T17:41:46Z

I extracted the option_given_p optimization in #622 and will merge it soon.

The create_id optimization was already merged in #454.

As for the rest of the PR, it look interesting but I'm struggling to rebase it.

@luke-gru I'm sorry to ask, but you think you can rebase/recreate?

Extracted from: ruby#512 Use `rb_hash_lookup2` to check for hash key existence instead of going through `rb_funcall`. Co-Authored-By: lukeg <luke.gru@gmail.com>

Extracted from: ruby/json#512 Use `rb_hash_lookup2` to check for hash key existence instead of going through `rb_funcall`. ruby/json@43835a0d13 Co-Authored-By: lukeg <luke.gru@gmail.com>

Closes: ruby#512 Co-Authored-By: lukeg <luke.gru@gmail.com>

byroot · 2024-10-18T14:17:18Z

but I'm struggling to rebase it.

Nevermind. After looking at it with fresh eyes, it was actually quite simple. All the parts of this PR have now been merged.

…s nil Closes: ruby/json#512 ruby/json@d882a45d82 Co-Authored-By: lukeg <luke.gru@gmail.com>

luke-gru force-pushed the remove_unnecessary_funcall_calls branch 2 times, most recently from cbd1224 to 32a2f66 Compare January 29, 2023 14:52

luke-gru force-pushed the remove_unnecessary_funcall_calls branch 2 times, most recently from 877eed0 to b6185b5 Compare January 30, 2023 22:58

Make JSON::Parser initialization faster

e525bed

Remove calls to rb_funcall, don't go back in to interpreter if not necessary. Also remove unnecessary calls to rb_respond_to(val) when val is nil. Speedup of JSON.parse when parsing small amount of data in a tight loop is up to 40%.

luke-gru force-pushed the remove_unnecessary_funcall_calls branch from b6185b5 to e525bed Compare January 30, 2023 23:06

nobu reviewed Feb 7, 2023

View reviewed changes

JSON parser init commit ruby#2 (temporary)

574c577

byroot added a commit to byroot/json that referenced this pull request Oct 17, 2024

Speedup Parser initialization

0bce099

Extracted from: ruby#512 Use `rb_hash_lookup2` to check for hash key existence instead of going through `rb_funcall`. Co-Authored-By: lukeg <luke.gru@gmail.com>

byroot mentioned this pull request Oct 17, 2024

Speedup Parser initialization #622

Merged

byroot added a commit to byroot/json that referenced this pull request Oct 17, 2024

Speedup Parser initialization

43835a0

Extracted from: ruby#512 Use `rb_hash_lookup2` to check for hash key existence instead of going through `rb_funcall`. Co-Authored-By: lukeg <luke.gru@gmail.com>

byroot force-pushed the master branch from ddee95a to 54363cb Compare October 18, 2024 09:52

casperisfine pushed a commit to casperisfine/json that referenced this pull request Oct 18, 2024

Ext::Parser avoid costly check on decimal_class when it is nil

d882a45

Closes: ruby#512 Co-Authored-By: lukeg <luke.gru@gmail.com>

casperisfine mentioned this pull request Oct 18, 2024

Ext::Parser avoid costly check on decimal_class when it is nil #628

Merged

byroot closed this in #628 Oct 18, 2024

hsbt pushed a commit to hsbt/ruby that referenced this pull request Oct 26, 2024

[ruby/json] Ext::Parser avoid costly check on decimal_class when it i…

b8471e4

…s nil Closes: ruby/json#512 ruby/json@d882a45d82 Co-Authored-By: lukeg <luke.gru@gmail.com>

hsbt pushed a commit to hsbt/ruby that referenced this pull request Oct 26, 2024

[ruby/json] Ext::Parser avoid costly check on decimal_class when it i…

f6d3467

…s nil Closes: ruby/json#512 ruby/json@d882a45d82 Co-Authored-By: lukeg <luke.gru@gmail.com>

hsbt pushed a commit to ruby/ruby that referenced this pull request Oct 26, 2024

[ruby/json] Ext::Parser avoid costly check on decimal_class when it i…

07fc21c

…s nil Closes: ruby/json#512 ruby/json@d882a45d82 Co-Authored-By: lukeg <luke.gru@gmail.com>

		if (json->decimal_class != Qnil) {
		if (rb_respond_to(json->decimal_class, i_try_convert)) {

-        if (json->decimal_class != Qnil) {
-        if (rb_respond_to(json->decimal_class, i_try_convert)) {
+        if (NIL_P(json->decimal_class)) {
+            /* use the default conversion */
+        } else if (rb_respond_to(json->decimal_class, i_try_convert)) {

Make JSON::Parser initialization faster #512

Make JSON::Parser initialization faster #512

Uh oh!

Conversation

luke-gru commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-gru commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mensfeld commented Jan 29, 2023

Uh oh!

hsbt commented Jan 30, 2023

Uh oh!

luke-gru commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsbt commented Jan 30, 2023

Uh oh!

luke-gru commented Jan 30, 2023

Uh oh!

nobu Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nobu Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Watson1978 Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-gru Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eregon Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

luke-gru Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

hsbt commented Feb 7, 2023

Uh oh!

mensfeld commented Mar 21, 2023

Uh oh!

luke-gru commented Mar 21, 2023

Uh oh!

luke-gru commented Apr 3, 2023

Uh oh!

mensfeld commented May 8, 2024

Uh oh!

byroot commented Oct 17, 2024

Uh oh!

byroot commented Oct 18, 2024

Uh oh!

Uh oh!

luke-gru commented Jan 29, 2023 •

edited

Loading

luke-gru commented Jan 29, 2023 •

edited

Loading

luke-gru commented Jan 30, 2023 •

edited

Loading

Watson1978 Feb 12, 2023 •

edited

Loading

luke-gru Feb 12, 2023 •

edited

Loading