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.

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

Replace ElasticArray with SmallVec #282

Merged
merged 19 commits into from
Dec 19, 2019
Merged

Replace ElasticArray with SmallVec #282

merged 19 commits into from
Dec 19, 2019

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Dec 12, 2019

The elastic-array crate is a fine piece of software but it'd be good to prefer crates that are commonly used in the ecosystem and likely to be well maintained. smallvec is at v1.0 and generally well-regarded.

Unfortunately as DBValue is part of the public API of kvdb, this is a breaking change so changing this implies annoying busy-work in consuming code.

@dvdplm dvdplm self-assigned this Dec 12, 2019
dvdplm added a commit to openethereum/parity-ethereum that referenced this pull request Dec 12, 2019
@dvdplm
Replace ElasticArray with SmallVec
dc945f0 
The elastic-array crate is a fine piece of software but it'd be good to prefer crates that are commonly used in the ecosystem and likely to be well maintained. smallvec is at v1.0 and generally well-regarded.

Related PRs:

	- [parity-common #282](paritytech/parity-common#282)
	– [trie-db #46](paritytech/trie#46)
@dvdplm dvdplm mentioned this pull request Dec 12, 2019
Closed
dvdplm
dvdplm commented Dec 15, 2019
View reviewed changes
parity-util-mem/src/impls.rs Outdated Show resolved Hide resolved
dvdplm
dvdplm commented Dec 15, 2019
View reviewed changes
kvdb/src/lib.rs Show resolved Hide resolved
dvdplm
dvdplm commented Dec 16, 2019
View reviewed changes
parity-util-mem/src/impls.rs Outdated Show resolved Hide resolved
parity-util-mem/src/impls.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm marked this pull request as ready for review December 17, 2019 10:48
@dvdplm dvdplm requested review from cheme and ordian December 17, 2019 10:48
ordian
ordian reviewed Dec 17, 2019
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looks good, just minor grumbles

kvdb-rocksdb/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb/CHANGELOG.md Show resolved Hide resolved
parity-util-mem/src/impls.rs Outdated Show resolved Hide resolved
parity-util-mem/src/impls.rs Show resolved Hide resolved
parity-util-mem/Cargo.toml
ethereum-types = { version = "0.8.0", optional = true, path = "../ethereum-types" }
parking_lot = { version = "0.9.0", optional = true }

[target.'cfg(target_os = "windows")'.dependencies]
winapi = "0.3.8"
winapi = { version = "0.3.8", features = ["heapapi"] }
Copy link
Member

Choose a reason for hiding this comment

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

so it didn't work before on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly don't know how it could have worked; looks like the winapi went completely crazy with features at some point and didn't care much for semver.
The risks of updating dependencies with cargo upgrade...

dvdplm and others added 2 commits December 17, 2019 13:22
@dvdplm @ordian
Update kvdb-rocksdb/CHANGELOG.md
420cfaf 
Co-Authored-By: Andronik Ordian <write@reusable.software>
@dvdplm
address review grumbles
60347a8
@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 17, 2019

@cheme I think it'd be good to have a somewhat coordinated release of trie-db before we can use this in eth/substrate. Can you help with that?

@cheme
Copy link
Collaborator

cheme commented Dec 17, 2019

I think we should include paritytech/trie#46 and paritytech/trie#39. I would also like to have paritytech/trie#45, but I did not find time to review it (but it can be released later).
For the release it probably touch more or less all trie crates, there was a versioning shift with the bench crate (0.17 instead of 0.16), so I would propose to solve that by publishing 0.18 (skip a version).
I am currently off, but if needed I probably can find some time this evening to release it (IIRC there is also parity-common triehash crate to update after publishing 0.18 trie crates).

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 17, 2019

I would propose to solve that by publishing 0.18 (skip a version).

Sounds good.

Also: we'd need a 👍 on this PR to proceed. :)

cheme
cheme approved these changes Dec 17, 2019
View reviewed changes
Copy link
Collaborator

@cheme cheme left a comment

Choose a reason for hiding this comment

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

👍

@dvdplm dvdplm mentioned this pull request Dec 17, 2019
Closed
@dvdplm dvdplm merged commit ae7abe2 into master Dec 19, 2019
@dvdplm dvdplm deleted the dp/chore/replace-elastic-array branch December 19, 2019 11:15
dvdplm added a commit that referenced this pull request Dec 19, 2019
@dvdplm
Merge branch 'master' into dp/feature/add-num-entries-to-rocksdb
88a3215 
* master:
  Replace ElasticArray with SmallVec (#282)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ordian ordian ordian approved these changes

@cheme cheme cheme approved these changes

Assignees

@dvdplm dvdplm

Labels
A0-pleasereview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdplm @cheme @ordian