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

feat: use lru instead of cached in near-network #5512

Merged
merged 1 commit into from
Dec 3, 2021
Merged

feat: use lru instead of cached in near-network #5512

merged 1 commit into from
Dec 3, 2021

Conversation

pmnoxx
Copy link
Contributor

@pmnoxx pmnoxx commented Nov 28, 2021 •
edited
Loading

use lru instead of cached in near-network

Reasons:

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 29, 2021 

Cargo.lock:170:1
--
  | [2021-11-28T21:00:22Z]     │
  | [2021-11-28T21:00:22Z] 170 │ ╭ hashbrown 0.11.2 registry+https://github.com/rust-lang/crates.io-index
  | [2021-11-28T21:00:22Z] 171 │ │ hashbrown 0.9.1 registry+https://github.com/rust-lang/crates.io-index
  | [2021-11-28T21:00:22Z]     │ ╰─────────────────────────────────────────────────────────────────────^ lock entries

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 29, 2021

This conflicts with

├── cached v0.23.0     
│   │   ├── hashbrown v0.9.1 (*)
-------------------------------------------------                                                                                  
│   │   │   └── serde_json v1.0.68                                                                                                                                                            
│   │   │       ├── indexmap v1.6.1                                                                                                                                                           
│   │   │       │   ├── hashbrown v0.9.1 (*)  
------------------------
│   │   ├── borsh v0.9.1
│   │   │   └── hashbrown v0.9.1

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 29, 2021

Downgrading lru to 0.6.5 to try to resolve version conflict.

@pmnoxx pmnoxx self-assigned this Nov 29, 2021
@pmnoxx pmnoxx marked this pull request as ready for review November 29, 2021 02:34
@pmnoxx pmnoxx requested a review from matklad November 29, 2021 02:34
@pmnoxx pmnoxx added the P-low Priority: low label Nov 29, 2021
@matklad
Copy link
Contributor

matklad commented Nov 29, 2021

Downgrading lru to 0.6.5 to try to resolve version conflict.

Ah, that's unfortunate. But yeah, I feel it's better to not pull yet another copy of hashbrown.

Fixing this properly (by updating, rather than downgrading) would require near/borsh-rs#45, near/borsh-rs#46, and removing cached altogether.

matklad
matklad approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Yeah, lru looks much, much more reasonable than sized cache. When this is merged, I'll update contract runtime usage of cached s well.

chain/network/Cargo.toml Outdated Show resolved Hide resolved
nearcore/src/config.rs Outdated Show resolved Hide resolved
@pmnoxx pmnoxx force-pushed the piotr-use-lru-instead-of-cached branch 2 times, most recently from 1c96961 to acc1052 Compare November 29, 2021 19:28
This was referenced Nov 29, 2021
Closed
Merged
bowenwang1996
bowenwang1996 reviewed Dec 1, 2021
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Question: are we confident in the lru implementation?

@matklad
Copy link
Contributor

matklad commented Dec 1, 2021

Without carefully reading every line of code, I am personally more confident in lru than in cached, as that looks like just a more tastefully design crate. lru's internal impl looks reasonable on the first glance, though it does makes sense from someone from core to give it a more comprehensive reading.

I've also noticed an opportunity for optimization there: jeromefroe/lru-rs#119. Trying to implement that might be a fun way to audit the crate.

@pmnoxx pmnoxx force-pushed the piotr-use-lru-instead-of-cached branch from acc1052 to f192942 Compare December 3, 2021 01:39
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Dec 3, 2021

@mm-near I looked at the implementation of lru. It looks reasonable. I can be optimized further, but it's faster than cached implementation.

@pmnoxx pmnoxx mentioned this pull request Dec 3, 2021
Merged
1 task
@pmnoxx pmnoxx force-pushed the piotr-use-lru-instead-of-cached branch from 064cad0 to d3f9a32 Compare December 3, 2021 02:23
@near-bulldozer near-bulldozer bot merged commit df4123e into master Dec 3, 2021
@near-bulldozer near-bulldozer bot deleted the piotr-use-lru-instead-of-cached branch December 3, 2021 02:39
near-bulldozer bot pushed a commit that referenced this pull request Dec 3, 2021
@pmnoxx
feat: Implement SyncLruCache (#5259)
81fb9ee 
This is just a proof of concept.

Implement a cache that works from a "&" context.

- get_from_cache_or_insert - gets a value or inserts a computed value
- get_from_cache - tries to get value 


TODO:
- [x] rename methods
- [x] add new methods
- [x] benchmarks
- [x] comments
- [x] add more tests.
- [x] one example of replacing real code
- [x] implement example with `lru` (done in another PR)
- [x] Use `once_cell` instead of `lazy_static!` (done in another PR). 

See #5145

Was blocked by: #5512
pmnoxx added a commit that referenced this pull request Dec 5, 2021
@pmnoxx
feat: Implement SyncLruCache (#5259)
45ca9e4 
This is just a proof of concept.

Implement a cache that works from a "&" context.

- get_from_cache_or_insert - gets a value or inserts a computed value
- get_from_cache - tries to get value

TODO:
- [x] rename methods
- [x] add new methods
- [x] benchmarks
- [x] comments
- [x] add more tests.
- [x] one example of replacing real code
- [x] implement example with `lru` (done in another PR)
- [x] Use `once_cell` instead of `lazy_static!` (done in another PR).

See #5145

Was blocked by: #5512
pmnoxx added a commit that referenced this pull request Dec 5, 2021
@pmnoxx
feat: Implement SyncLruCache (#5259)
fd0dca9 
This is just a proof of concept.

Implement a cache that works from a "&" context.

- get_from_cache_or_insert - gets a value or inserts a computed value
- get_from_cache - tries to get value

TODO:
- [x] rename methods
- [x] add new methods
- [x] benchmarks
- [x] comments
- [x] add more tests.
- [x] one example of replacing real code
- [x] implement example with `lru` (done in another PR)
- [x] Use `once_cell` instead of `lazy_static!` (done in another PR).

See #5145

Was blocked by: #5512
pmnoxx added a commit that referenced this pull request Dec 5, 2021
@pmnoxx
feat: Implement SyncLruCache (#5259)
a375551 
This is just a proof of concept.

Implement a cache that works from a "&" context.

- get_from_cache_or_insert - gets a value or inserts a computed value
- get_from_cache - tries to get value

TODO:
- [x] rename methods
- [x] add new methods
- [x] benchmarks
- [x] comments
- [x] add more tests.
- [x] one example of replacing real code
- [x] implement example with `lru` (done in another PR)
- [x] Use `once_cell` instead of `lazy_static!` (done in another PR).

See #5145

Was blocked by: #5512
pmnoxx added a commit that referenced this pull request Dec 5, 2021
@pmnoxx
feat: Implement SyncLruCache (#5259)
a3b08ef 
This is just a proof of concept.

Implement a cache that works from a "&" context.

- get_from_cache_or_insert - gets a value or inserts a computed value
- get_from_cache - tries to get value

TODO:
- [x] rename methods
- [x] add new methods
- [x] benchmarks
- [x] comments
- [x] add more tests.
- [x] one example of replacing real code
- [x] implement example with `lru` (done in another PR)
- [x] Use `once_cell` instead of `lazy_static!` (done in another PR).

See #5145

Was blocked by: #5512
@pmnoxx pmnoxx added A-network Area: Network C-dependencies Category: Pull requests that update a dependency file C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality labels Dec 5, 2021
near-bulldozer bot pushed a commit that referenced this pull request Dec 8, 2021
@pmnoxx
feat: Fix near_network::RoutingTableView::get_announce performance (…
1dd1c1d 
…#5595)

While reading `get_announce` implementation, I noticed that for every `AnnounceAccount` read, we were writing it back to disk.
Let's simplify `get_announce` to avoid additional disk writes.

Based on top of #5512

TODO after this PR:
- [ ]  more cleanup of `RoutingTableView`


Testing:
We have tests for this code
@matklad matklad mentioned this pull request Dec 15, 2021
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad approved these changes

@bowenwang1996 bowenwang1996 bowenwang1996 approved these changes

@mm-near mm-near Awaiting requested review from mm-near

Assignees

@pmnoxx pmnoxx

Labels
A-network Area: Network C-dependencies Category: Pull requests that update a dependency file C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pmnoxx @matklad @bowenwang1996