refactor: deduplicate integer serialization in RollingBloom benchmark #24088

phyBrackets · 2022-01-17T18:34:33Z

refactor: deduplicate integer serialization in RollingBloom benchmark and i think we have to follow don't repeat yourself pattern here! I don't think that this much affect the memory and efficiency because it is still in constant time . But surely it reduces some readability of code which is not the big headache for sure.

theStack

Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling:

bitcoin/src/crypto/common.h

Lines 44 to 48 in d0bf9bb

    
           void static inline WriteLE32(unsigned char* ptr, uint32_t x) 
        
           { 
        
               uint32_t v = htole32(x); 
        
               memcpy(ptr, (char*)&v, 4); 
        
           }

bitcoin/src/crypto/common.h

Lines 77 to 81 in d0bf9bb

    
           void static inline WriteBE32(unsigned char* ptr, uint32_t x) 
        
           { 
        
               uint32_t v = htobe32(x); 
        
               memcpy(ptr, (char*)&v, 4); 
        
           }

As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

phyBrackets · 2022-01-17T19:09:59Z

Thanks for the suggestion. I update the following.
@theStack

sipa · 2022-01-17T21:27:52Z

src/bench/rollingbloom.cpp

-        data[1] = (count >> 8) & 0xFF;
-        data[2] = (count >> 16) & 0xFF;
-        data[3] = (count >> 24) & 0xFF;
+        for(int i = 0; i <= 3; i++){


This is just WriteLE32(data, count);.

sipa · 2022-01-17T21:28:21Z

src/bench/rollingbloom.cpp

-        data[1] = (count >> 16) & 0xFF;
-        data[2] = (count >> 8) & 0xFF;
-        data[3] = count & 0xFF;
+        for(int i = 3; i >= 0; i--){


This is just WriteBE32(data, count);.

so, should i change the following to just a function call by adding common.h header ?

Yeah, better than duplicating the logic here.

fanquake · 2022-01-18T02:38:23Z

Once you've addressed the suggestions, make sure you squash your changes to a single commit, with a proper commit message.

phyBrackets · 2022-01-18T06:29:42Z

Once you've addressed the suggestions, make sure you squash your changes to a single commit, with a proper commit message.

@fanquake will take care of that. Thanks.

phyBrackets · 2022-01-18T20:43:12Z

Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling:

bitcoin/src/crypto/common.h

Lines 44 to 48 in d0bf9bb

void static inline WriteLE32(unsigned char* ptr, uint32_t x)

{

uint32_t v = htole32(x);

memcpy(ptr, (char*)&v, 4);

}

bitcoin/src/crypto/common.h

Lines 77 to 81 in d0bf9bb

void static inline WriteBE32(unsigned char* ptr, uint32_t x)

{

uint32_t v = htobe32(x);

memcpy(ptr, (char*)&v, 4);

}

As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

Isn't this bit inefficient because we are actually dropping the whole content of common.h ?

phyBrackets · 2022-02-04T20:29:10Z

Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling:

bitcoin/src/crypto/common.h

Lines 44 to 48 in d0bf9bb

void static inline WriteLE32(unsigned char* ptr, uint32_t x)

{

uint32_t v = htole32(x);

memcpy(ptr, (char*)&v, 4);

}

bitcoin/src/crypto/common.h

Lines 77 to 81 in d0bf9bb

void static inline WriteBE32(unsigned char* ptr, uint32_t x)

{

uint32_t v = htobe32(x);

memcpy(ptr, (char*)&v, 4);

}

As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

Isn't this bit inefficient because we are actually dropping the whole content of common.h ?

Hey @sipa what's your views on this before making a function call ?

sipa · 2022-02-04T20:37:46Z

I don't understand your question. Using ReadLE32/WriteLE32 is almost certainly more efficient than reimplementing it yourself (they're written in a way that the compiler can easily optimize out). Not that ~nanosecond level performance is relevant here...

phyBrackets · 2022-02-04T21:02:48Z

oh , okay ! Then it's fine I think.

phyBrackets · 2022-02-04T21:25:49Z

Sorry for the direct commit , need to do some more changes.

phyBrackets · 2022-02-12T14:54:15Z

Hey, any update on this?

maflcko · 2022-02-12T15:02:35Z

I haven't looked at the changes, but this can't be merged unless squashed.

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

phyBrackets · 2022-02-12T19:13:48Z

Cool, will do that soon. Thanks!

phyBrackets · 2022-02-14T14:22:00Z

Hey, I'm having some problem with squashing the commits. Should I make new PR with final changes? Will close this and refer to this PR! If you would allow?

fanquake · 2022-02-14T14:28:43Z

Should I make new PR with final changes?

No. Please don't open new PRs for the same changes. You might need to take another look through: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits/.

phyBrackets · 2022-02-17T18:36:09Z

Hey @fanquake , I tried to rebase/squash , I have some problems with branches I think , there is branch conflict . I really don't want to open a new PR but it seems the only option , by the way if you would allow, I'll open that and set the reference to this PR , and will close this pr ?

sipa · 2022-02-17T18:43:59Z

@phyBrackets It's probably worthwhile learning how to resolve these issues anyway, if you want to continue contributing, whether you do that for this PR or a future one.

src/bench/rollingbloom.cpp

phyBrackets · 2022-02-18T17:31:40Z

@phyBrackets It's probably worthwhile learning how to resolve these issues anyway, if you want to continue contributing, whether you do that for this PR or a future one.

Hey @sipa , I'm really trying and searching around how to resolve this issue but I'm not able to , actually when I' trying to run git rebase -i HEAD~7 , this only shows the first 4 commits.

maflcko · 2022-02-19T08:29:06Z

Your branch contains merge commits, which can't be squashed in a rebase.

Something like this might work (untested!!):

git checkout patch-1                                       # Verify this is on commit aa91fd400a6a23feeacdff0d2adacd18dd313e47
git fetch origin 5d254a234d8c1569b0161264cc6d5d8d0ce0d864  # Fetch the latest master
git merge 5d254a234d8c1569b0161264cc6d5d8d0ce0d864         # Merge the latest master
git reset --soft 5d254a234d8c1569b0161264cc6d5d8d0ce0d864  # Discard commit history, but keep all changes staged
git commit --message="bla bla"                             # Create a commit with all staged changes
git push origin patch-1 --force

phyBrackets · 2022-02-19T09:27:18Z

I tried this , it seems more mess tho.

maflcko · 2022-02-19T09:34:33Z

After the steps in #24088 (comment) you need to force push, not merge

phyBrackets · 2022-02-19T09:43:54Z

Thank you sm @MarcoFalke . It seems like it works fine.

theStack

Code-review ACK e1fcf8d

Nit: I don't think the dataPtr pointer is really needed here, you could just call the serialization helper functions with data.data() as first parameter each.

phyBrackets · 2022-02-21T05:56:07Z

Cool, but I think it is more readable through this . But yeah if this affects on unnecessary memory use. We can remove that. Let me know your further take on this.

laanwj · 2022-04-06T11:22:37Z

Nit: I don't think the dataPtr pointer is really needed here, you could just call the serialization helper functions with data.data() as first parameter each.

+1. The assignment is unnecessary. I doubt it affects memory use in any significant way but I see no reason not to write it shorter if you're touching this code anyway.

fanquake · 2022-04-06T13:07:47Z

I've just gone ahead and fixed this up in #24784, given this is straightforward, and likely been open long enough.

…loom benchmark fff9141 refactor: Remove deduplication of data in rollingbloom bench (phyBrackets) Pull request description: Fixed up #24088. ACKs for top commit: vincenzopalazzo: ACK fff9141 Tree-SHA512: 9fef617bceb74a1aec4f4a1e7c4732c4764af3e8ac2fc02b84ce370e8b97431957ca17ee8f44fb96765f7304f8d7e5bfb951440db98ba40f240612f2232d215e

…ollingBloom benchmark fff9141 refactor: Remove deduplication of data in rollingbloom bench (phyBrackets) Pull request description: Fixed up bitcoin#24088. ACKs for top commit: vincenzopalazzo: ACK bitcoin@fff9141 Tree-SHA512: 9fef617bceb74a1aec4f4a1e7c4732c4764af3e8ac2fc02b84ce370e8b97431957ca17ee8f44fb96765f7304f8d7e5bfb951440db98ba40f240612f2232d215e

theStack reviewed Jan 17, 2022

View reviewed changes

phyBrackets changed the title ~~shouldn't we follow DRY principle here?~~ refactor: deduplicate integer serialization in RollingBloom benchmark Jan 17, 2022

phyBrackets closed this Jan 17, 2022

phyBrackets reopened this Jan 17, 2022

DrahtBot added the Refactoring label Jan 17, 2022

sipa reviewed Jan 17, 2022

View reviewed changes

phyBrackets force-pushed the patch-1 branch from 3cc1172 to d7825aa Compare February 5, 2022 07:24

maflcko reviewed Feb 17, 2022

View reviewed changes

src/bench/rollingbloom.cpp Outdated Show resolved Hide resolved

Remove the deduplicating of data in rollingbloom.cpp

e1fcf8d

phyBrackets force-pushed the patch-1 branch from fb74d41 to e1fcf8d Compare February 19, 2022 09:42

theStack approved these changes Feb 20, 2022

View reviewed changes

fanquake mentioned this pull request Apr 6, 2022

refactor: deduplicate integer serialization in RollingBloom benchmark #24784

Merged

fanquake closed this Apr 6, 2022

bitcoin locked and limited conversation to collaborators Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: deduplicate integer serialization in RollingBloom benchmark #24088

refactor: deduplicate integer serialization in RollingBloom benchmark #24088

phyBrackets commented Jan 17, 2022 •

edited

Loading

theStack left a comment

phyBrackets commented Jan 17, 2022

sipa Jan 17, 2022

sipa Jan 17, 2022

phyBrackets Jan 18, 2022

sipa Feb 4, 2022 •

edited

Loading

fanquake commented Jan 18, 2022

phyBrackets commented Jan 18, 2022

phyBrackets commented Jan 18, 2022

phyBrackets commented Feb 4, 2022

sipa commented Feb 4, 2022

phyBrackets commented Feb 4, 2022

phyBrackets commented Feb 4, 2022

phyBrackets commented Feb 12, 2022

maflcko commented Feb 12, 2022

phyBrackets commented Feb 12, 2022

phyBrackets commented Feb 14, 2022

fanquake commented Feb 14, 2022

phyBrackets commented Feb 17, 2022

sipa commented Feb 17, 2022

phyBrackets commented Feb 18, 2022

maflcko commented Feb 19, 2022 •

edited

Loading

phyBrackets commented Feb 19, 2022

maflcko commented Feb 19, 2022

phyBrackets commented Feb 19, 2022

theStack left a comment

phyBrackets commented Feb 21, 2022

laanwj commented Apr 6, 2022

fanquake commented Apr 6, 2022

	void static inline WriteLE32(unsigned char* ptr, uint32_t x)
	{
	uint32_t v = htole32(x);
	memcpy(ptr, (char*)&v, 4);
	}

	void static inline WriteBE32(unsigned char* ptr, uint32_t x)
	{
	uint32_t v = htobe32(x);
	memcpy(ptr, (char*)&v, 4);
	}

refactor: deduplicate integer serialization in RollingBloom benchmark #24088

refactor: deduplicate integer serialization in RollingBloom benchmark #24088

Conversation

phyBrackets commented Jan 17, 2022 • edited Loading

theStack left a comment

Choose a reason for hiding this comment

phyBrackets commented Jan 17, 2022

sipa Jan 17, 2022

Choose a reason for hiding this comment

sipa Jan 17, 2022

Choose a reason for hiding this comment

phyBrackets Jan 18, 2022

Choose a reason for hiding this comment

sipa Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

fanquake commented Jan 18, 2022

phyBrackets commented Jan 18, 2022

phyBrackets commented Jan 18, 2022

phyBrackets commented Feb 4, 2022

sipa commented Feb 4, 2022

phyBrackets commented Feb 4, 2022

phyBrackets commented Feb 4, 2022

phyBrackets commented Feb 12, 2022

maflcko commented Feb 12, 2022

phyBrackets commented Feb 12, 2022

phyBrackets commented Feb 14, 2022

fanquake commented Feb 14, 2022

phyBrackets commented Feb 17, 2022

sipa commented Feb 17, 2022

phyBrackets commented Feb 18, 2022

maflcko commented Feb 19, 2022 • edited Loading

phyBrackets commented Feb 19, 2022

maflcko commented Feb 19, 2022

phyBrackets commented Feb 19, 2022

theStack left a comment

Choose a reason for hiding this comment

phyBrackets commented Feb 21, 2022

laanwj commented Apr 6, 2022

fanquake commented Apr 6, 2022

phyBrackets commented Jan 17, 2022 •

edited

Loading

sipa Feb 4, 2022 •

edited

Loading

maflcko commented Feb 19, 2022 •

edited

Loading