Skip to content

Avoid allocating channels unnecessarily on map inserts#3246

Merged
peterebden merged 5 commits intothought-machine:masterfrom
peterebden:avoid-chan-alloc
Sep 10, 2024
Merged

Avoid allocating channels unnecessarily on map inserts#3246
peterebden merged 5 commits intothought-machine:masterfrom
peterebden:avoid-chan-alloc

Conversation

@peterebden
Copy link
Copy Markdown
Collaborator

@peterebden peterebden commented Sep 1, 2024

We don't always insert, but we were always allocating a new slice just in case we did. This makes it lazy.

I don't love duplicating the Set function but I don't see a great alternative (it also hurts a bit to add the extra non-inlineable function call on every insert when we do have a value).

Before:

INFO: Run 1 of 5
INFO: Complete in 12.06s, using 14470100 KB
INFO: Run 2 of 5
INFO: Complete in 12.64s, using 15571780 KB
INFO: Run 3 of 5
INFO: Complete in 12.14s, using 14686056 KB
INFO: Run 4 of 5
INFO: Complete in 13.47s, using 15443652 KB
INFO: Run 5 of 5
INFO: Complete in 12.06s, using 13783684 KB
INFO: Complete, median time: 12.14s, median mem: 14686056.00 KB

After:

INFO: Run 1 of 5
INFO: Complete in 11.94s, using 14640700 KB
INFO: Run 2 of 5
INFO: Complete in 12.00s, using 13965888 KB
INFO: Run 3 of 5
INFO: Complete in 11.68s, using 14098944 KB
INFO: Run 4 of 5
INFO: Complete in 12.32s, using 15145640 KB
INFO: Run 5 of 5
INFO: Complete in 12.15s, using 14744944 KB
INFO: Complete, median time: 12.00s, median mem: 14640700.00 KB

Comment thread src/cmap/cmap.go Outdated
s.m[key] = awaitableValue[V]{Val: f()}
close(existing.Wait)
existing.Wait = nil
return existing.Val, true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be s.m[key].Val? IIUC, existing.Val will be zero if existing.Wait is non-nil.

It looks like the existing tests don't actually check the return value of Set (and I question whether it actually needs to return the value at all)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Set was supposed to return the old value; there'd be no point in returning s.m[key].Val there since that's what the caller supplied.

I think you're right that nothing actually cares about this any more though, so maybe it can all just be simplified out.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okay right, I've removed it from Set where it was no longer needed. This now returns whatever is set now which is more useful (the callers of it never look at the value in that code path but it still makes more sense this way).

Copy link
Copy Markdown
Contributor

@toastwaffle toastwaffle left a comment

Choose a reason for hiding this comment

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

plz fix lint, otherwise lgtm

@peterebden peterebden merged commit 8471691 into thought-machine:master Sep 10, 2024
@peterebden peterebden deleted the avoid-chan-alloc branch September 10, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants