Skip to content

compressor: use a prevector in CompressScript serialization [ZAP1] #18847

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2021
Merged

compressor: use a prevector in CompressScript serialization [ZAP1] #18847

merged 1 commit into from
Apr 28, 2021

Conversation

jb55
Copy link
Contributor

@jb55 jb55 commented May 2, 2020 •
edited
Loading

This function was doing millions of unnecessary heap allocations during IBD.

I'm start to catalog unnecessary heap allocations as a pet project of mine: as-zero-as-possible-alloc IBD. This is one small step.

before:
May01-174536

after:
May01-174610

should I type alias this? I type aliased it

This is a part of the Zero Allocations Project #18849 (ZAP1). This code came up as a place where many allocations occur.

@DrahtBot DrahtBot added the Tests label May 2, 2020
@jb55 jb55 mentioned this pull request May 2, 2020
Closed
4 tasks
@Empact
Copy link
Contributor

Empact commented May 2, 2020

There's also a call in src/test/fuzz/script.cpp

bitcoin/src/test/fuzz/script.cpp

Lines 39 to 40 in 844d207

std::vector<unsigned char> compressed;
if (CompressScript(script, compressed)) {

Empact
Empact reviewed May 2, 2020
View reviewed changes
@sipa
Copy link
Member

sipa commented May 3, 2020

Any reason not to do this for decompression?

@jb55
Copy link
Contributor Author

jb55 commented May 3, 2020

Any reason not to do this for decompression?

nope, I could do that as well.

@jb55 jb55 force-pushed the 2020-05-compresscript-prevector branch 4 times, most recently from 3ac60ed to 14cd3d9 Compare May 15, 2020 18:34
Empact
Empact reviewed May 15, 2020
View reviewed changes
@jb55 jb55 force-pushed the 2020-05-compresscript-prevector branch 3 times, most recently from 0616c3d to d03a694 Compare May 15, 2020 19:30
JeremyRubin
JeremyRubin reviewed May 15, 2020
View reviewed changes
@jb55 jb55 force-pushed the 2020-05-compresscript-prevector branch 2 times, most recently from b782989 to 98c0688 Compare May 15, 2020 21:47
JeremyRubin
JeremyRubin reviewed May 15, 2020
View reviewed changes
@jb55
Copy link
Contributor Author

jb55 commented May 15, 2020 via email

@jb55 jb55 force-pushed the 2020-05-compresscript-prevector branch 2 times, most recently from 30834be to 6183bda Compare May 15, 2020 22:25
@jb55
compressor: use a prevector in compressed script serialization
83a425d 
Use a prevector for stack allocation instead of heap allocation during
script compression and decompression. These functions were doing
millions of unnecessary heap allocations during IBD.

We introduce a CompressedScript type alias for this prevector. It is
size 33 as that is the maximum size of a compressed script.

Fix the DecompressScript header to match the variable name from
compressor.cpp

Signed-off-by: William Casarin <jb55@jb55.com>
@jb55 jb55 force-pushed the 2020-05-compresscript-prevector branch from 6183bda to 83a425d Compare May 15, 2020 22:27
@jb55 jb55 changed the title compressor: use prevector in CompressScript serialization compressor: use a prevector in CompressScript serialization May 15, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented May 16, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@jb55 jb55 changed the title compressor: use a prevector in CompressScript serialization compressor: use a prevector in CompressScript serialization [ZAP1] May 16, 2020
@Empact
Copy link
Contributor

Empact commented May 16, 2020

ACK 83a425d

@elichai
Copy link
Contributor

elichai commented May 17, 2020

tACK 83a425d

@jb55
Copy link
Contributor Author

jb55 commented May 21, 2020

doing some tests I'm seeing a very tiny improvement in tmpfs/memdisk reindex up to height 200k:

but could just be luck 🤷 would be interesting to test this to even higher heights.

n       Δn=6      Δn+1      ms       
1       -3135    -581       219706    zeroalloc-mem-200k
2       -2554    -981       220287    zeroalloc-mem-200k
3       -1573    -100       221268    zeroalloc-mem-200k
4       -1473    -88        221368    master-mem-200k
5       -1385    -1385      221456    master-mem-200k
6                           222841    master-mem-200k

I also traced the number of calls to CompressScript: around 57248988 calls at height 200k

I used

@jb55
Copy link
Contributor Author

jb55 commented Apr 27, 2021

friendly yearly bump

@sipa
Copy link
Member

sipa commented Apr 28, 2021

utACK 83a425d

@maflcko maflcko merged commit 4cfe6c3 into bitcoin:master Apr 28, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 29, 2021
Merge bitcoin#18847: compressor: use a prevector in CompressScript se…
a47fad5 
…rializa…
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 4cfe6c3 into merged_master (Bitcoin PR bitcoin/bitcoin#18847)
10ee572
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@Empact Empact Empact left review comments

@JeremyRubin JeremyRubin JeremyRubin left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jb55 @Empact @sipa @DrahtBot @elichai @JeremyRubin @maflcko