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

C++ benchmark is inaccurate #1

Open
zig-for opened this issue Apr 12, 2018 · 7 comments
Open

C++ benchmark is inaccurate #1

zig-for opened this issue Apr 12, 2018 · 7 comments
Labels
benchmark bug Something isn't working

Comments

@zig-for
Copy link

zig-for commented Apr 12, 2018

https://github.com/btwael/SuperString/blob/master/test/withStd.cc#L26 this line always copies from the current character to the very end of the file (rather than just to the end of the line). Also, the last line is skipped. The memory used should be much less than 450MB.

@zig-for zig-for changed the title C++ benchmark is wrong. C++ benchmark is inaccurate Apr 12, 2018
@btwael btwael added bug Something isn't working benchmark labels Apr 12, 2018
@btwael btwael closed this as completed in #3 Apr 12, 2018
@fcortes
Copy link

fcortes commented Apr 12, 2018

I think this should be reopened as the benchmark are indeed still inaccurate. I actually couldn't replicate the results claimed in the readme but I'm not sure if my setup is completely correct. I had to add the for(auto _ : state) in the bench_compare.cc file to get any statistic from the benchmark library, and the execution times where about 60x slower for the SuperString implementation :/ The number of CPU iteration was smaller though

@zig-for
Copy link
Author

zig-for commented Apr 12, 2018

@btwael btwael reopened this Apr 12, 2018
@btwael
Copy link
Owner

btwael commented Apr 12, 2018

In a last test, std::string consumed 11MB, while supersrring consumed 10MB. for the moment, don't use superstring with small amount of data. Rope structure are more performant with heavy text manipulation.

Also the library may have better benchmark result in 32bit than 64bit machine

@btwael
Copy link
Owner

btwael commented Apr 12, 2018

I have to admit that the api difference between std::string::substr(startIndex, length) and SuperString::substring(startIndex, endInex) tricked me, and many reviews from reddit and hacker news show how code quality can be improved, you can also remark that there is no big difference between std::string and SuperString (in general usage, may steel useful for big manipulation).

The ideas behind SuperString are amazing, but the usage of a lot of pointers (8 bytes in x64, that's 8 ASCII character) kills the advantage of rope data structures.

So I will try to implement additional data representation for short manipulation (e.g. if you the result of substring (number of character) is less than the substring structure (pointer to the original string, startIndex, endIndex)) just copy the data.

Thank you for being interested, and sorry because my bar plot was tricky and inaccurate, I will try to fix that as soon as possible.

I want also to refers this blog post that inspired me to create SuperString (the blog post is very accurate)

@zig-for
Copy link
Author

zig-for commented Apr 14, 2018

Try doing an operation with a lot of concatenations - it may show off the power of your library better.

@ghost
Copy link

ghost commented Apr 17, 2018

The misleading benchmarks should be removed until they are replaced with more accurate ones.

@btwael
Copy link
Owner

btwael commented Apr 18, 2018

The misleading benchmarks are removed, the development is focused on making it compatible with std::string so we can test in real-world applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants