Skip to content

codec/linearcodec: remove "SkipRegistration" as TODO, simplify locks #1482

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
Closed
gyuho wants to merge 4 commits into from
Closed

codec/linearcodec: remove "SkipRegistration" as TODO, simplify locks #1482

gyuho wants to merge 4 commits into from

Conversation

@gyuho
Copy link
Contributor

@gyuho gyuho commented May 6, 2023

Why this should be merged

We should never call SkipRegistration after initialization. Just remove it from the interface, and make linearcodec.New more generic.

How this works

Add options args, to consolidate linear codec creation.

How this was tested

UT

@gyuho
codec/linearcodec: remove "SkipRegistration", simplify locks
1d2d534 
Signed-off-by: Gyuho Lee <gyuho.lee@avalabs.org>
@gyuho gyuho self-assigned this May 6, 2023
@gyuho
Copy link
Contributor Author

gyuho commented May 6, 2023

../../../go/pkg/mod/github.com/ava-labs/coreth@v0.12.1-rc.0/plugin/evm/codec.go:27:4: c.SkipRegistrations undefined (type linearcodec.Codec has no field or method SkipRegistrations)

:)

gyuho added 2 commits May 6, 2023 17:35
@gyuho
linearcodec: add "SkipRegistration"
09bcdad 
Signed-off-by: Gyuho Lee <gyuho.lee@avalabs.org>
@gyuho
note DEPRECATION
5ee580e 
Signed-off-by: Gyuho Lee <gyuho.lee@avalabs.org>
@gyuho gyuho changed the title codec/linearcodec: remove "SkipRegistration", simplify locks codec/linearcodec: remove "SkipRegistration" as TODO, simplify locks May 6, 2023
@StephenButtolph
Copy link
Contributor

StephenButtolph commented May 6, 2023

We should never call SkipRegistration after initialization

Why not? I think it makes total sense to support skipping type IDs

@gyuho
Copy link
Contributor Author

gyuho commented May 6, 2023

support skipping type IDs

I only see SkipRegistration in init (at least in avalanchego)? If you see a use case we may call after initialization, I will close the PR :)

My initial attempt was to minimize the write locks, but RegisterType needs write locks anyways...

hexfusion pushed a commit to hexfusion/avalanchego that referenced this pull request Jun 22, 2023
@StephenButtolph
Convert cumulative cpu to cpu since last measurement (ava-labs#1482)
bec0d24
@github-actions
Copy link

github-actions bot commented Jun 25, 2023

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

@github-actions
Copy link

github-actions bot commented Sep 10, 2023

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

@StephenButtolph StephenButtolph deleted the linear-codec-simplify branch July 24, 2024 20:46
maru-ava pushed a commit that referenced this pull request Dec 3, 2025
@RodrigoVillar
refactor(db/revisionmanager): move RootStore creation to `RevisionM…
ec187ba 
…anager` (#1482)

In preparation of moving in-memory revisions to their own data structure
(#1470), this PR moves creation of `RootStore` into the
`RevisionManager` - this move will allow us to inject the in-memory
revisions data structure into `RootStore` during creation.

This PR is a precursor to #1480.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abi87 abi87 Awaiting requested review from abi87

@StephenButtolph StephenButtolph Awaiting requested review from StephenButtolph StephenButtolph is a code owner

@danlaine danlaine Awaiting requested review from danlaine

@coffeeavax coffeeavax Awaiting requested review from coffeeavax

Assignees

@gyuho gyuho

Labels

lifecycle/stale

Projects

No open projects
Ski Patrol
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gyuho @StephenButtolph @aaronbuchwald @abi87