New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve json range support #1906

Merged
merged 2 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@John-Colvin
Contributor

John-Colvin commented Aug 30, 2017

A very simple translation of skipNumber to the range API.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Aug 30, 2017

Contributor

@John-Colvin please profile the performance consequences of this change.
It seems like it'd hurt quite a bit. please profile with -Os as well

Contributor

UplinkCoder commented Aug 30, 2017

@John-Colvin please profile the performance consequences of this change.
It seems like it'd hurt quite a bit. please profile with -Os as well

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 31, 2017

Member

One small performance improvement would be to skip incrementing idx for ranges that define length and use takeExactly(sOrig.length - s.length) instead. But benchmarking is a topic that I need to get back to anyway. I did a quick test at DConf this year and performance was really bad, whereas I remember pretty good numbers from the last tests.

Member

s-ludwig commented Aug 31, 2017

One small performance improvement would be to skip incrementing idx for ranges that define length and use takeExactly(sOrig.length - s.length) instead. But benchmarking is a topic that I need to get back to anyway. I did a quick test at DConf this year and performance was really bad, whereas I remember pretty good numbers from the last tests.

@John-Colvin

This comment has been minimized.

Show comment
Hide comment
@John-Colvin

John-Colvin Aug 31, 2017

Contributor

The major performance problem is now fixed by operating on ubyte[] instead of auto-decoded strings (Oh how I hate them).

In debug mode assumeUTF verifies utf, which is of course expensive. In release, that check doesn't happen.

I would have preferred to use byCodeUnit instead, but dmd isn't so great at optimising that.

Contributor

John-Colvin commented Aug 31, 2017

The major performance problem is now fixed by operating on ubyte[] instead of auto-decoded strings (Oh how I hate them).

In debug mode assumeUTF verifies utf, which is of course expensive. In release, that check doesn't happen.

I would have preferred to use byCodeUnit instead, but dmd isn't so great at optimising that.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 15, 2017

Member

Thanks, John! The performance implication of the additional index is supposedly small enough.

The more important question IMO is anyway to see what the future of std_data_json is. There were some points brought up that made it difficult to continue the Phobos integration route, so it might make sense to establish it as an independent library and let it slowly replace this module.

Member

s-ludwig commented Sep 15, 2017

Thanks, John! The performance implication of the additional index is supposedly small enough.

The more important question IMO is anyway to see what the future of std_data_json is. There were some points brought up that made it difficult to continue the Phobos integration route, so it might make sense to establish it as an independent library and let it slowly replace this module.

@s-ludwig s-ludwig merged commit a67bf9f into vibe-d:master Sep 15, 2017

4 checks passed

codecov/patch 80.434% of diff hit (target 46.185%)
Details
codecov/project 49.021% (+2.836%) compared to bc706b2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@John-Colvin John-Colvin deleted the John-Colvin:skip_number_range branch Sep 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment