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

immediate Updates and lazy cost evaluation #75

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Conversation

karlmcguire
Copy link
Contributor

@karlmcguire karlmcguire commented Sep 27, 2019

This PR addresses the Set/Del race condition by putting Dels on the same internal buffer, as well as immediately updating Set values and eventually updating the corresponding cost.

Costs can also now be calculated using the config.Coster function.


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Comments.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jarifibrahim and @karlmcguire)


cache.go, line 98 at r1 (raw file):

	// is ran after Set is called for a new item or an item update with a cost
	// param of 0.
	Coster func(value interface{}) int64

Cost


cache.go, line 188 at r1 (raw file):

		return false
	}
	i := &item{itemNew, c.keyToHash(key), value, cost}

and here as well (see below)


cache.go, line 189 at r1 (raw file):

	}
	i := &item{itemNew, c.keyToHash(key), value, cost}
	// attempt to immediately update hashmap value and set flag to update so the

Attempt to


cache.go, line 194 at r1 (raw file):

		i.flag = itemUpdate
	}
	// attempt to send item to policy

Attempt to


cache.go, line 209 at r1 (raw file):

		return
	}
	c.setBuf <- &item{itemDelete, c.keyToHash(key), nil, 0}
&item{
  flag: ...,
  hash: ...,
}

cache.go, line 219 at r1 (raw file):

	for i := range c.setBuf {
		// calculate item cost value if new or update
		if (i.flag == itemNew || i.flag == itemUpdate) && i.cost == 0 {

i.cost == 0 && c.coster && i.flag != itemDelete


policy.go, line 192 at r1 (raw file):

func (p *defaultPolicy) Update(key uint64, cost int64) {
	p.Lock()
	defer p.Unlock()

Avoid a defer here.


policy.go, line 198 at r1 (raw file):

func (p *defaultPolicy) Cost(key uint64) int64 {
	p.Lock()
	defer p.Unlock()

cost, found = ...
p.Unlock()
if found { return cost }
return -1


policy.go, line 244 at r1 (raw file):

func (p *sampledLFU) updateIfHas(key uint64, cost int64) (updated bool) {
	if prev, exists := p.keyCosts[key]; exists {
		// Update the cost of the existing key. For simplicity, don't worry about evicting anything

Keep the comment.


store.go, line 111 at r1 (raw file):

func (m *lockedMap) Update(key uint64, value interface{}) bool {
	m.Lock()
	defer m.Unlock()

Take a sweep across the entire code base. And see what all defers you can get rid of.

@ben-manes
Copy link

Is there a reason you prefer cost and Coster versus the Java terms (weight and Weigher)?(When refer you referred to the entry's size in your article, "a heavy item could displace many lightweight items", you used weight as the metaphor)

@karlmcguire
Copy link
Contributor Author

I was just going to change it to Cost, but I like the idea of using Weigher for the reason you mentioned... I will check with @manishrjain.

Copy link
Contributor Author

@karlmcguire karlmcguire left a comment

Choose a reason for hiding this comment

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

:lgtm:

Dismissed @manishrjain from 10 discussions.
Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @jarifibrahim and @manishrjain)


cache.go, line 98 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Cost

Done.


cache.go, line 188 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

and here as well (see below)

Done.


cache.go, line 189 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Attempt to

Done.


cache.go, line 194 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Attempt to

Done.


cache.go, line 209 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…
&item{
  flag: ...,
  hash: ...,
}

Done.


cache.go, line 219 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

i.cost == 0 && c.coster && i.flag != itemDelete

Done.


policy.go, line 192 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Avoid a defer here.

Done.


policy.go, line 198 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

cost, found = ...
p.Unlock()
if found { return cost }
return -1

Done.


policy.go, line 244 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Keep the comment.

Done.


store.go, line 111 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Take a sweep across the entire code base. And see what all defers you can get rid of.

Done.

Copy link
Contributor Author

@karlmcguire karlmcguire left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jarifibrahim)

@karlmcguire karlmcguire merged commit d963fa2 into master Oct 1, 2019
@karlmcguire karlmcguire deleted the karl/update branch October 1, 2019 14:22
cache.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants