-
Notifications
You must be signed in to change notification settings - Fork 622
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.
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
refactor: improve compilation time for neard tests #4263
Conversation
See near#4027 for discussion why this layout is better. TL;DR is that we don't re-link neard repeatedly. As a cherry on top, we now get to proprely share NodeCluster code between the tests, instead of it being compiled afresh for each test.
Implementation wise, replace a global lazy_static_state with local once_cell state. As a bonus, that gets rid of a Mutex
Hehe, turns out that this is not trivial -- there was a bit of a global state, which caused interverence between the tests in the same process. The last commit removes that global state. |
@bowenwang1996 or @evgenykuzyakov , could you take another look at this commit: d8d0fd8 ? It's a bit more than "just move the code around", as we need to fix deps between the tests as well. |
pub fn new(genesis_runtime_config: RuntimeConfig) -> Self { | ||
Self { | ||
genesis_runtime_config: Arc::new(genesis_runtime_config), | ||
with_lower_storage_cost: OnceCell::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively just go ahead and construct the alternative configuration. It would increase memory usage if the protocol feature is not used but simplify things (and minimally speed things up) for the case of the feature being enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's an extremely reasonable suggestion, to say the least! There's really no need for any kind of laziness here.
For what its worth, LGTM. |
@matklad any objections to merging this PR? |
This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks. |
I still want to merge this, but the specific change no longer is the best way to fix this after https://github.com/near/nearcore/pull/4381/files#r657012158. Let me close this and create an issue instead. |
See #4027 for discussion why this layout is better. TL;DR is that we
don't re-link neard repeatedly.
As a cherry on top, we now get to proprely share NodeCluster code
between the tests, instead of it being compiled afresh for each test.