GADTs + inline records for more efficient doubly linked lists#58
Open
mmottl wants to merge 1 commit intojanestreet:masterfrom
Open
GADTs + inline records for more efficient doubly linked lists#58mmottl wants to merge 1 commit intojanestreet:masterfrom
mmottl wants to merge 1 commit intojanestreet:masterfrom
Conversation
|
See comment on #59 |
|
@mmottl, would you mind rebasing you PR ? |
012dd8b to
f752279
Compare
Author
|
@hhugo No problem, it's rebased now. |
|
This seems like a totally sensible improvement. Is there any reason why we haven't imported this feature? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch implements the same kind of efficiency improvement as the one recently submitted for the union-find algorithm. Though the patch touches many lines, the vast majority of changes are completely trivial and easily recognized as correct.
The patch will require the upcoming OCaml 4.04 (or beta) to show performance improvements. It eliminates an indirection and associated memory allocations. In particular, changes to the first element of the doubly linked list (e.g. the frequently used
insert_first) will now need one less allocation. Only thefirstfunction is slower now, which is likely insufficient reason not to use the patch.I don't have a comprehensive benchmark suite (you probably do), but in some simple tests (e.g.
insert_firstof a lot of elements + removal of each inserted element), the performance gain was around 2.5% and allocated words were reduced by about 10%.The speed improvement isn't overwhelming, mostly because the OCaml runtime is so crazy good at small allocations. But a reduction in allocated words will reduce GC pressure and possible latencies associated with more frequent collections.