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

Cheaper and faster prehash generation mechanism #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

szaydel
Copy link
Contributor

@szaydel szaydel commented Aug 3, 2018

I am not sure if you have much interest in this, but I found that this method of creating id is no less reliable and faster as well as cheaper.

I must admit I have not done exhaustive testing, but if you find this interesting I am happy to do a good bit more validation. In my limited testing it has been reliable.

Ran a few comparisons just so there's something I can show. The only difference is this changeset.

These are my two test scripts...

#!/bin/sh
echo "*** Run 1000 iterations new method ***"
for i in `seq 0 999`; do
    /home/ubuntu/bin/c.new "redis-h1get.c -ggdb -Wall -I/usr/include/hiredis `pkg-config --libs --cflags hiredis`" h1 tags > /dev/null
done
echo "*** Done running new method ***"
#!/bin/sh
echo "*** Run 1000 iterations original method ***"
for i in `seq 0 999`; do
    /home/ubuntu/bin/c "redis-h1get.c -ggdb -Wall -I/usr/include/hiredis `pkg-config --libs --cflags hiredis`" h1 tags > /dev/null
done
echo "*** Done original method ***"
*** Run 1000 iterations new method ***
*** Done running new method ***
3.56user 6.32system 0:38.73elapsed 25%CPU (0avgtext+0avgdata 3280maxresident)k
0inputs+16000outputs (0major+2885064minor)pagefaults 0swaps

*** Run 1000 iterations new method ***
*** Done running new method ***
3.48user 6.51system 0:38.55elapsed 25%CPU (0avgtext+0avgdata 3304maxresident)k
0inputs+16000outputs (0major+2890112minor)pagefaults 0swaps


*** Run 1000 iterations original method ***
*** Done original method ***
17.10user 19.30system 1:02.29elapsed 58%CPU (0avgtext+0avgdata 8440maxresident)k
0inputs+112000outputs (0major+3196581minor)pagefaults 0swaps

*** Run 1000 iterations original method ***
*** Done original method ***
16.76user 20.06system 1:01.91elapsed 59%CPU (0avgtext+0avgdata 8420maxresident)k
0inputs+112000outputs (0major+3196095minor)pagefaults 0swaps

Thanks!

@ryanmjacobs
Copy link
Owner

Nice! Yeah, that method looks a lot faster.

It seems that all tests are passing, so I don't see a reason not to accept this. I'll take a deeper look at the code itself tomorrow.

Nice work!

@szaydel
Copy link
Contributor Author

szaydel commented Aug 4, 2018

Sounds good. I am sure one can find that this method could be improved. Let me know if you find any issues you feel are non-starters, and I can see about addressing them as time allows.

And, thanks again!

@szaydel
Copy link
Contributor Author

szaydel commented Jul 5, 2019

@ryanmjacobs, I was wondering if we should just close this, or if you have interest still, I am happy to fix it, to make sure it could be merged in without any conflicts.

@ryanmjacobs
Copy link
Owner

Hi @szaydel, I'm okay with merging this as long as I'm 100% certain that it doesn't break anything.

In terms of efficiency, this bumps it down from ~17 ms to ~4 ms. Generally, if a UI element performs in less than 100 ms, nobody will notice anyways. I'm okay with sacrificing 15 ms for correctness.

That being said...

My primary concern is that this doesn't run the file through cpp (the C Preprocessor). For example, when you compile this code...

int main(void) {
#ifdef __unix__
  return 1;
#endif  

  return 0;
}

c should re-compile the code if __unix__ happens to be redefined. But without running through the C preprocessor, the hash will never change (because the source code technically never changed).

@szaydel
Copy link
Contributor Author

szaydel commented Jul 6, 2019

Fair point about the preprocessor. I guess I am not 100% certain that this does not break anything, at least in my testing it did not. Why don't we close this out for now, and I will play more with this to make sure I am very confident first, and then we can open this conversation up again.

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