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

Introduce generic linked list #2904

Closed
wants to merge 7 commits into from
Closed

Introduce generic linked list #2904

wants to merge 7 commits into from

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Apr 3, 2024

Why this should be merged

Improves the performance of linked.Hashmap#Put by ~50%.

Before:

Benchmark_Hashmap-12    	 6913144	       166.5 ns/op	      80 B/op	       2 allocs/op

After:

Benchmark_Hashmap-12    	13979869	        82.58 ns/op	       0 B/op	       0 allocs/op

How this works

Previously, the linked.Hashmap depended on the "container/list" library. This resulted in 2 memory allocations.

  1. For the creation of the keyValue struct that was wrapped into an any interface.
  2. For the creation of the list.Element struct enforced by the list functions.

By implementing our own version of the linked list, we are able to use generics to avoid the memory allocation for the interface conversion.

Additionally, our version of the linked list allows allocating the Element externally to the list. This allows the Hashmap to maintain a free list (which isn't an increase in memory pressure, because the map never frees allocated space either).

How this was tested

  • Added unit tests
  • Added Hashmap benchmark
  • CI

@StephenButtolph StephenButtolph self-assigned this Apr 3, 2024
@StephenButtolph StephenButtolph added the cleanup Code quality improvement label Apr 3, 2024
@StephenButtolph StephenButtolph added this to the v1.11.4 milestone Apr 3, 2024
@StephenButtolph StephenButtolph marked this pull request as ready for review April 3, 2024 05:12
@StephenButtolph StephenButtolph changed the title Linked list Introduce generic linked list Apr 3, 2024

// PushFront inserts e at the front of l.
// If e is already in a list, l is not modified.
func (l *List[T]) PushFront(e *ListElement[T]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is supposed to be a drop in replacement of containers/list, still let me ask a question.

  • Wouldn't it be more natural to have methods to add a value (PushFront/Back InsertBefore/After) accepting a value T instead of a ListElement[T]?
  • Wouldn't it be more idiomatic to signal "no next/prev element" via an error rather than a nil pointer? I guess here performance considerations may have prevailed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the methods take in ListElement rather than T to allow the caller to manage all memory allocations. I'll add some helpers to expose the API with allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from the ListElement vs T change, I kept the signature the same as the stdlib to allow ease of switching. (Which is why we use nil to signal no element rather than a bool.

Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

just nits and questions, but LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code quality improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants