Skip to content
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

Multiple implementation partial refactoring #197

Merged
merged 5 commits into from
Jul 2, 2019

Conversation

ioioioio
Copy link
Member

This partial refactoring (not done yet for stage2) has two goals: simplify the implementation of multiple versions for different instruction sets and to eventually allow runtime dispatching. It should facilitate the merging of PR#139 and the realization of issue#20.

The benchmark shows comparable performances to the last simdjson version. Not slower, not faster.

It has been tested on ARM and the results are the same as before.

However, the modifications made on parse.cpp and statisticalmodel.cpp already show the limits of such a new structure: it might become cumbersome if we eventually need more than one entry point.

Any suggestion is welcome.

@lemire
Copy link
Member

lemire commented Jun 28, 2019

@ioioioio Thank you.

@lemire
Copy link
Member

lemire commented Jun 28, 2019

I will review shortly. I will verify performance.

@lemire
Copy link
Member

lemire commented Jul 1, 2019

I have verified that the instruction count is nearly the same (+/- instruction) for both stage 1 and stage 2.

…#ifdef to select the right SIMD set all the time.

Fixing indentation.
Removing some obsolete WARN_UNUSED.
Fixing a weird warning with optind variable.
@lemire
Copy link
Member

lemire commented Jul 1, 2019

However, the modifications made on parse.cpp and statisticalmodel.cpp already show the limits of such a new structure: it might become cumbersome if we eventually need more than one entry point.

See my commit aa78b70 where I have a workaround for the ugliness: we default the template on a "native" instruction set. Essentially, I am pushing the #ifdef up into the library itself.

@lemire
Copy link
Member

lemire commented Jul 1, 2019

Note that this code moves much of the stage 1 implementation into a header file. There are ways to avoid this, but it is always a bit finicky whereas the "header file" approach always works. So I think what we have here is fine.

@lemire
Copy link
Member

lemire commented Jul 1, 2019

@ioioioio If you are fine with the result as it stands, please merge it into master at your earliest convenience.

@lemire lemire self-requested a review July 1, 2019 19:14
@lemire lemire assigned lemire and ioioioio and unassigned lemire Jul 1, 2019
Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to provide use with a good way to handle complexity and it opens the door to runtime dispatch.

@ioioioio ioioioio merged commit 78406ba into master Jul 2, 2019
@ioioioio ioioioio deleted the multiple_implementation_refactoring branch July 2, 2019 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants