Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 17, 2022

Identical commit, taken as-is from #25985

This reverts ee7b84e from bitcoin#20527.
This change was made without any rationale, maybe other than a brew
installed version might be newer, and that's "better". However when
building from source on macOS, it just results in drastically worse
perofrmance, and results in issues / confusions like bitcoin#25724.

Resolves the "build from source" portion of bitcoin#25724. Building from
depends is still not ideal, however I have some other changes that might
help improve things in that case.

The difference in performance can be observed using the example from
bitcoin#25724 (comment),
but minified to only 10 descriptors. i.e:
```bash
time src/bitcoin-cli createwallet speedy true
time src/bitcoin-cli importdescriptors '[
  {"desc":"raw(00145846369f3d6ba366d6f5a903fb5cf4dca3763c0e)#k9wh6v62","timestamp":"now"},
  {"desc":"raw(001420800aabf13f3a4c4ce3ce4c66cecf1d17f21a6e)#6m0hlfh4","timestamp":"now"},
  {"desc":"raw(0014c6bf9715e06d73ebf9b3b02d5cc48d24d8bbabc1)#wyavh36r","timestamp":"now"},
  {"desc":"raw(00141ba7807b3f46af113beaea5c698428ce7138cd8a)#jctdsups","timestamp":"now"},
  {"desc":"raw(00140c1bd27f10fff01b36ddf3c1febaa1acff19b080)#9s6nc3pk","timestamp":"now"},
  {"desc":"raw(00141226e31987e4bc2e63c0ee12908f675e40464b20)#9pp7qm39","timestamp":"now"},
  {"desc":"raw(0014f73f149f7503960a5e849c6ee7a8a8c336f631cb)#qtkxv9fc","timestamp":"now"},
  {"desc":"raw(0014c8ccb4d81ffc769fc5fdd8d7eed69b0e0cae5749)#hn39qayv","timestamp":"now"},
  {"desc":"raw(001498565aead2d67a22a6021d55210f2a917fc22169)#6ar3vwsx","timestamp":"now"},
  {"desc":"raw(001403013248ac0cd9eabe176cad162cda2a19f771e1)#4m47mukd","timestamp":"now"}
]'
```

Running master, when building from souce and using brew installed
sqlite, this takes ~3.4s. With this PR, the same operation takes ~0.3s.
@dergoegge
Copy link
Member

ACK d216d71

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK d216d71

nit: no usual commit message backport stuff like

Github-Pull: ...
Rebased-From: ...

?

@fanquake fanquake added this to the 24.0 milestone Oct 18, 2022
@maflcko
Copy link
Member Author

maflcko commented Oct 18, 2022

Yes, it was written before the branch-off, so it can just be merged like any other commit before the branch-off. Just seems extra effort to change the commit id, add metadata, and then ask reviewers to verify the metadata again for no reason?

@hebasto
Copy link
Member

hebasto commented Oct 18, 2022

Yes, it was written before the branch-off, so it can just be merged like any other commit before the branch-off.

Right.

@fanquake fanquake merged commit bb5bcf3 into bitcoin:24.x Oct 18, 2022
@fanquake fanquake deleted the revert_slow_macos_sqlite branch October 18, 2022 12:05
@bitcoin bitcoin locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants