⚡ Sangsakala Proxy 🏠 Home
Skip to content

What about using upstream RapidJSON 1.1? #42

New issue
New issue
Closed
Closed
What about using upstream RapidJSON 1.1?#42

Description

@lelit
lelit
opened on Aug 27, 2016

Hi Ken,

I spent some hours "rebasing" py-rapidjson on top of current RapidJSON version 1.1, released a few days ago.

The branch currently working ok, all tests pass; also, with a grain of salt, after a few iteration of the benchmarks, it seems almost always perform slightly better than current master. Where it is slower, its mainly caused by a few other impacting (hopefully) changes, that I may either try to smooth, or extract in a different branch, or forget about, at your option:

  • does not use native C++ "numbers" (ie floats and ints), but rather use the "raw value", to parse them thru {PyFloat|PyDecimal|PyLong}_FromString(): for this reason, I dropped the option "precise_floats"
  • I did not (yet?) rebased/cherrypicked your str optimizations to the writer you made here and here
  • I used a submodule to bring in upstream RapidJSON sources, which, expecially if you bless me on doing point 2 above, seems a better/cleaner solution rather than including just the sources as it was; it's of course up to you, I can just easily use subtree to include it while keeping its history/reference with the upstream sources, or simply copy version 1.1 upstream, like before, should you prefer that

Current state is in my rj-v1.1-as-submodule.

Am I on the right path? Could any parts of that branch be accepted as a PR?

Thank you!

Activity

  1. kenrobbins commented on Aug 29, 2016

    @kenrobbins
    kenrobbins
    on Aug 29, 2016
    Member

    I'm fine with using RapidJSON 1.1, especially since it looks like it has support for the custom changes I was using. It will also make it easier to upgrade again in the future, if necessary.

    As for submodule vs including source directly, I was originally going to use a submodule, but after I started modifying RapidJSON, I found it easier to just copy the source. If there is a better way to use a submodule and incorporate custom code, I'm all for that. It would be nice to allow for the custom changes, though, since making custom optimizations is the most fun part for me. 😃

  2. lelit commented on Aug 29, 2016

    @lelit
    lelit
    on Aug 29, 2016
    ContributorAuthor

    I just made several runs of my branch (rebased on current master), with and without your optimizations mentioned above, and was not able to determine which variant is faster... It would be slightly easier to compare them merging my benchmark branch (issue #24) 😄

    Wrt the submodule, what I suggest is forking RapidJSON v1.1 and point the submodule to your own fork. That way you can keep a clean separation between upstream and your custom changes, making it easier to integrate further future versions. The same can be done with subtree... if it were up to me I'd go with the submodule for now, we can easily change idea later.

    So, the first point remain: what do you think about using the RawNumber API and the drop of precise_float option? I could either try to bring it back introducing a new native_number option or something like that...

  3. kenrobbins commented on Aug 30, 2016

    @kenrobbins
    kenrobbins
    on Aug 30, 2016
    Member

    Re: optimizations. I'm okay leaving them out if it's not clear they're helping. Is there a PR related to #24? I can merge.

    Re: submodule. Good idea, that sounds better.

    Re: floats. I'm not attached to any particular implementation. My goal when writing this lib was speed, but I guess I don't have any specific guidelines about how to balance speed and features. What do you think?

  4. lelit commented on Aug 30, 2016

    @lelit
    lelit
    on Aug 30, 2016
    ContributorAuthor

    Ken Robbins notifications@github.com writes:

    Re: optimizations. I'm okay leaving them out if it's not clear they're
    helping. Is there a PR related to #24? I can merge.

    Great, I will rebase that branch on current master and open a PR for it!

    Re: submodule. Good idea, that sounds better.

    Good.

    Re: floats. I'm not attached to any particular implementation. My goal when
    writing this lib was speed, but I guess I don't have any specific guidelines
    about how to balance speed and features. What do you think?

    I'm biased toward speed, but only when it does not unconditionally force
    arbitrary limits. Nowadays we are all using 64bit archs so it may sound silly,
    but consider:

    >>> import json, rapidjson
>>> rapidjson.dumps(sys.maxsize*10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: int too big to convert
>>> json.dumps(sys.maxsize*10)
'92233720368547758070'

    I would prefer having a switch to state that I'm obsessed by speed but I'm
    also sure that all my math is confined between arch's hard limits 😄

    That would also agree with the saying "first do it right, then optimize, if
    needed".

  5. kenrobbins commented on Aug 31, 2016

    @kenrobbins
    kenrobbins
    on Aug 31, 2016
    Member

    Makes sense to me. I'm convinced.

  6. lelit commented on Aug 31, 2016

    @lelit
    lelit
    on Aug 31, 2016
    ContributorAuthor

    Great then, I will rebase the branch on current master, update the docs and open an PR.

  7. kenrobbins
    added a commit that references this issue on Dec 8, 2016

    Merge pull request #46 from lelit/rj-v1.1-as-submodule-v2

    5405613

  8. sontek commented on Dec 9, 2016

    @sontek
    sontek
    on Dec 9, 2016
    Contributor

    +1 @kenrobbins just merged this PR so this issue can be closed

  9. kenrobbins commented on Dec 9, 2016

    @kenrobbins
    kenrobbins
    on Dec 9, 2016
    Member

    thanks

  10. kenrobbins
    closed this as completedon Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions