Add stream projection API #171

keithduncan · 2017-02-26T11:12:38Z

This ports the pure Ruby stream projection API I built previously http://github.com/keithduncan/json-projection to the yajl C API. Using the lexer we can project the stream of JSON tokens and only allocate the Ruby objects we actually care about. This is much more performant than the pure Ruby version.

Comparing performance

Taking a sample GitHub push event webhook push.json, the intended use case here's how it performs comparing speed and space.

Benchmark.benchmark

                                          user     system      total        real
JSON.parse                            0.000000   0.000000   0.000000 (  0.008270)
JsonProjection::Projector.project     0.020000   0.000000   0.020000 (  0.033700)
Yajl::Projector                       0.000000   0.000000   0.000000 (  0.005155)

Benchmark.memory

Calculating -------------------------------------
          JSON.parse    98.623k memsize (     0.000  retained)
                       590.000  objects (     0.000  retained)
                        50.000  strings (     0.000  retained)
JsonProjection::Projector.project
                         1.049M memsize (     0.000  retained)
                        22.873k objects (     0.000  retained)
                        50.000  strings (     0.000  retained)
     Yajl::Projector    14.542k memsize (     0.000  retained)
                       190.000  objects (     0.000  retained)
                        50.000  strings (     0.000  retained)

Comparison:
     Yajl::Projector:      14542 allocated
          JSON.parse:      98623 allocated - 6.78x more
JsonProjection::Projector.project:    1048893 allocated - 72.13x more

For my sample push.json which is 10kB Yajl::Projector is the fastest and allocates the least memory.

Now taking push-big.json which is a webhook I artificially inflated not having one of our giant pushes to hand, real world performance still to be determined:

Benchmark.benchmark

                                          user     system      total        real
JSON.parse                            0.100000   0.010000   0.110000 (  0.140367)
JsonProjection::Projector.project    11.880000   0.040000  11.920000 ( 11.930685)
Yajl::Projector                       0.050000   0.010000   0.060000 (  0.086963)

Benchmark.memory

Calculating -------------------------------------
          JSON.parse    34.709M memsize (     0.000  retained)
                       180.171k objects (     0.000  retained)
                        50.000  strings (     0.000  retained)
     Yajl::Projector     2.920M memsize (     0.000  retained)
                        47.301k objects (     0.000  retained)
                        50.000  strings (     0.000  retained)

Comparison:
     Yajl::Projector:    2919930 allocated
          JSON.parse:   34709329 allocated - 11.89x more

I can’t profile JsonProjection::Projector for memory because the Ruby process dies 😱 meanwhile Yajl::Projector is 1.6x faster than JSON.parse and allocates 11.89x less memory.

event buffers are fixed size but point into bigger buffers

brianmario · 2017-02-27T06:46:04Z

This is insane, and I LOVE it. I'm currently pretty sick and would like to give this a much better review when my head clears in a few days. But I'm loving what I've seen so far!

In a perfect world I would have moved yajl-ruby on top of the latest version of yajl itself before we did this, but the bundled version is already somewhat diverged from upstream anyway.

yajl 2.0 brings features I added in the bundled version here (of yajl 1.x) with a couple of exceptions. It's been a while since I last looked at upgrading. But I certainly wouldn't consider it a blocker against this amazingness.

I'll get back to this soon, I promise! <3

mistydemeo · 2017-11-30T02:52:16Z

@brianmario Just wanted to check in - I have a project using this branch of yajl-ruby, and it'd be really useful to have this in master or (especially!) in a new tagged release!

brianmario

Took a pass, things are looking pretty good to me! I'd love to possibly get a review from @tenderlove and/or @charliesome for the C api usage (in case there are any gotchas I'm unaware of, specifically around the usage of RB_GC_GUARD).

Also, this is making me think about JSON Schema. Which, while a seemingly much heavier-weight, seemingly similar solution - I'm curious of the advantages of disadvantages to what we have here?

brianmario · 2017-12-11T05:54:04Z

ext/yajl/yajl_ext.c

+
+    while (1) {
+        if (parser->offset >= RSTRING_LEN(parser->buffer)) {
+            //printf("reading offset %d size %ld\n", parser->offset, RSTRING_LEN(parser->buffer));


Would you mind cleaning this up? :)

brianmario · 2017-12-11T05:54:22Z

ext/yajl/yajl_ext.c

+
+        yajl_tok token;
+        if (pop == 0) {
+            //printf("peeking %p %ld %d\n", RSTRING_PTR(parser->buffer), RSTRING_LEN(parser->buffer), parser->offset);


brianmario · 2017-12-11T05:54:28Z

ext/yajl/yajl_ext.c

+        if (pop == 0) {
+            //printf("peeking %p %ld %d\n", RSTRING_PTR(parser->buffer), RSTRING_LEN(parser->buffer), parser->offset);
+            token = yajl_lex_peek(parser->lexer, (const unsigned char *)RSTRING_PTR(parser->buffer), RSTRING_LEN(parser->buffer), parser->offset);
+            //printf("peeked event %d\n", token);


brianmario · 2017-12-11T05:54:48Z

ext/yajl/yajl_ext.c

+            return event;
+        }
+
+        //printf("popping\n");


brianmario · 2017-12-11T05:54:54Z

ext/yajl/yajl_ext.c

+
+        //printf("popping\n");
+        token = yajl_lex_lex(parser->lexer, (const unsigned char *)RSTRING_PTR(parser->buffer), RSTRING_LEN(parser->buffer), &parser->offset, (const unsigned char **)&event.buf, &event.len);
+        //printf("popped event %d\n", token);


brianmario · 2017-12-11T05:56:48Z

ext/yajl/yajl_ext.c

+# Given the start of an array or object, read until the closing event.
+# Object structures can nest and this is considered.
+#
+# Returns nothing.


Same with using * here instead of #.

brianmario · 2017-12-11T05:58:16Z

ext/yajl/yajl_ext.c

+                val = rb_float_new(strtod(buf, NULL));
+            } else {
+                val = rb_cstr2inum(buf, 10);
+            }


Let's make sure rb_float_new and rb_cstr2inum can't ever raise, otherwise we'll leak buf.

brianmario · 2017-12-11T05:59:02Z

ext/yajl/yajl_ext.c

+        }
+
+        case yajl_tok_string_with_escapes:; {
+            //printf("decoding string with escapes\n");


More cleanup plz :)

brianmario · 2017-12-11T06:01:29Z

spec/projection/project_file.rb

+      x.compare!
+    }
+  end
+end


Could we move this to the top-level benchmark dir? Just so we don't take up this time during the tests (not that it's a huge deal...)

brianmario · 2017-12-11T06:01:44Z

spec/projection/projection.rb

+      ]
+    }.to_json
+
+    puts json


Do we need this puts here?

tenderlove · 2017-12-12T20:59:03Z

ext/yajl/yajl_ext.c

+            //printf("reading offset %d size %ld\n", parser->offset, RSTRING_LEN(parser->buffer));
+
+            // Refill the buffer
+            rb_funcall(parser->stream, intern_io_read, 2, INT2FIX(RSTRING_LEN(parser->buffer)), parser->buffer);


Does this need an rb_protect in case the read call raises an exception? I'm not sure how this function fits in to the rest of the program yet, but we might need to clean up memory or something if there's an error.

tenderlove · 2017-12-12T21:02:19Z

ext/yajl/yajl_ext.c

+
+        // nil schema means reify the subtree from here on
+        // otherwise if the schema has a key for this we want it
+        int interesting = (schema == Qnil || rb_funcall(schema, rb_intern("key?"), 1, key) == Qtrue);


Do we support schemas that aren't hashes? If not, I think we should use rb_hash_has_key here.

tenderlove · 2017-12-12T21:21:15Z

ext/yajl/yajl_ext.c

+            val = rb_yajl_projector_build_simple_value(parser, value_event);
+        }
+
+        rb_hash_aset(hsh, key, val);


I think we might want to call rb_str_freeze on the key here. Otherwise rb_hash_aset will dup and freeze the key.

tenderlove · 2017-12-12T21:33:37Z

This is amazing! I think we can get better performance by doing the key freeze I mentioned earlier.

Another idea for more performance: it seems we allocate a Ruby string even if we aren't interested in the key (we have to generate the string so we can call key? on the hash). Maybe we could convert the schema to st_table structures. Then we could check to see if the key is "interesting" without allocating a Ruby object. Of course we'd have to generate that structure, but we should only have to do the generation once per schema, and then we could amortize the cost of generation.

tenderlove

I think I spotted a small leak that we should fix

tenderlove · 2017-12-12T21:58:28Z

ext/yajl/yajl_ext.c

+        .lexer = yajl_lex_alloc(&allocFuncs, 0, 1),
+    };
+
+    VALUE result = rb_yajl_projector_filter_subtree(&parser, schema, yajl_event_stream_next(&parser, 1));


rb_yajl_projector_filter_subtree can raise an exception. In which case the lexer won't get freed. We should move this around a little to fix the leak.

brianmario · 2017-12-12T23:34:53Z

Going to go ahead and merge this so we can iterate on it without needing to deal with forks.

keithduncan added 30 commits February 25, 2017 15:30

Add projection spec

ad82dc3

Create the object shell

39f2dcd

Create a hash

0694bf4

Make a static hash to pass the test

8d4d9eb

Start porting the event stream

c21ad60

Call the subtree instead of the static object

b13fcae

Fetch the parser out of the instance variable

7349fc4

Try the event loop

573cf50

Move assertions

46de339

Printf debugging

b5095f8

Fix the read message dispatch

5f969ab

Implement build_subtree

8365128

Log the event

269b874

Fix compiling

91467ee

Ignore commas when iterating the hash

dfadfa5

Add failing spec for filtering

870b90f

Comment the event numbers

cc0cd7e

Raise exceptions

b8d1cf6

Implement the filtering?

9edf245

These return void

0ce2a02

No bool type womp womp

2de4555

Semicolons+++++

6c0c050

Fix warning with a cast

b976d32

Remove ISO C90 warnings

11344e2

Fix all the warnings!!

5ad1fb3

Port Array spec from json-projection

03c44dc

Add filters subtree tests

56b871a

Fix false tokens

19f4b05

Raise with context when a rogue boolean token is received

0065112

Use memcmp rather than strcmp

f1918a9

event buffers are fixed size but point into bigger buffers

keithduncan and others added 21 commits February 26, 2017 14:48

Add pending test that we passthrough utf8

0e7e1f0

Add failing spec for UTF8 passthrough

df36b00

Fix encoding of non escape sequence strings

39a0afe

Update the object test names for utf8 passthrough and utf16 escapes

f61760b

Add tests for unicode object keys

5cd9dff

Add a wrapper function to convert string like events into rb strings

a3307a8

Add test for utf8 passthrough of object values

7f90afa

Add spec for projecting a file

99d670d

Comment out the prints

8d560e3

Fix typo

50923db

Add benchmarking

9f35f9a

Add memory benchmark

489c090

Add failing spec for ignoring escaped strings

30a42ae

Fix spec by ignoring escaped strings

9d9b121

Make this the actual UTF-16 encoding

120db9b

Add failing spec for hashes whose second key is an escaped string

444ed3b

Fix failing spec

3b90742

Add tok name API

040bfe6

Print the token name that was read instead of the int

b816f78

Merge branch 'remove-old-rubies' into stream-projection

f6a82ad

Bump version to 1.4.0

ca6674a

Merge branch 'master' into stream-projection

59aca65

brianmario reviewed Dec 11, 2017

View reviewed changes

tenderlove reviewed Dec 12, 2017

View reviewed changes

tenderlove requested changes Dec 12, 2017

View reviewed changes

brianmario merged commit 76f7462 into brianmario:master Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stream projection API #171

Add stream projection API #171

keithduncan commented Feb 26, 2017

brianmario commented Feb 27, 2017

mistydemeo commented Nov 30, 2017

brianmario left a comment

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

brianmario Dec 11, 2017

tenderlove Dec 12, 2017

tenderlove Dec 12, 2017

tenderlove Dec 12, 2017

tenderlove commented Dec 12, 2017

tenderlove left a comment

tenderlove Dec 12, 2017

brianmario commented Dec 12, 2017

Add stream projection API #171

Add stream projection API #171

Conversation

keithduncan commented Feb 26, 2017

Comparing performance

brianmario commented Feb 27, 2017

mistydemeo commented Nov 30, 2017

brianmario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenderlove commented Dec 12, 2017

tenderlove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmario commented Dec 12, 2017