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.

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

perf(core): Introduce mutable map struct #9199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

harshil-goel
Copy link
Contributor

@harshil-goel harshil-goel commented Oct 16, 2024

Currently the delta postings are being stored inside a map. When we are using cache, these deltas gets unnecessarily copied again and again because of the limitations of map. We have introduced a new struct, that would allow us to work on the list of
deltas.
With the introduction of this, we can now without copying the map, can pass around lists from one transaction to another without issue.

We are also going to store results like delete markers, uid to posting map and other stuff required to make queries go faster.

@harshil-goel harshil-goel requested a review from a team as a code owner October 16, 2024 13:56
@github-actions github-actions bot added area/testing Testing related issues area/core internal mechanisms go Pull requests that update Go code labels Oct 16, 2024
@harshil-goel harshil-goel force-pushed the harshil-goel/mutable-map branch 2 times, most recently from 6fdd230 to 2f48d14 Compare October 21, 2024 18:53
@harshil-goel harshil-goel changed the title perf(core): Introduce mutable map struct to update mutation map in Li… perf(core): Introduce mutable map struct Oct 23, 2024
posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated

mm.curTime = ts
mm.curList = pl
mm.uidMap = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

mm.uidMap.clear()?

posting/list.go Outdated
}

for ts, pl := range mm.oldList {
if mm.curTime != 0 && ts > mm.curTime {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this. We should just merge with the below if

posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated
mm.length = 0
}
if mpost.Op == Del {
mm.length -= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there is only deleteAll and nothing else? Can the length be -1 then?

posting/list.go Outdated Show resolved Hide resolved
posting/list.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core internal mechanisms area/testing Testing related issues go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

4 participants