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

Make it run faster. #205

Draft
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

make-github-pseudonymous-again
Copy link
Member

The goal is to take care of #120 and #108.

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021

Codecov Report

Merging #205 (b14478c) into main (96a58a6) will increase coverage by 0.90%.
The diff coverage is 99.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
+ Coverage   98.20%   99.10%   +0.90%     
==========================================
  Files          49       61      +12     
  Lines        1282     2469    +1187     
==========================================
+ Hits         1259     2447    +1188     
+ Misses         23       22       -1     
Impacted Files Coverage Δ
src/0-core/concatenate/index.js 100.00% <ø> (ø)
src/0-core/index.js 100.00% <ø> (ø)
src/1-digit/index.js 100.00% <ø> (ø)
src/4-lazy/Lazy.js 91.01% <91.01%> (ø)
src/1-digit/4-Four.js 97.40% <98.35%> (+0.69%) ⬆️
src/0-core/_fast/_append_small_list.js 100.00% <100.00%> (ø)
src/0-core/_fast/_fill_right.js 100.00% <100.00%> (ø)
src/0-core/_fast/_from_by_filling.js 100.00% <100.00%> (ø)
src/0-core/_fast/_from_medium_list.js 100.00% <100.00%> (ø)
src/0-core/_fast/_prepend_small_list.js 100.00% <100.00%> (ø)
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a58a6...b14478c. Read the comment docs.

We immediately instantiate a Digit or null. It's a win-win best deal ever:
  - If we need a Digit, it's there!
  - No need to branch on list.length! So we win even when we need a Tree.

Also we use separate struct DigitSplit to store result of _splitDigit since
member types are not the same as in a tree Split. Maybe should rename Split to
TreeSplit later.
This introduces more non-pure methods which can only be used with care.
Currently these are prefixed with _UNSAFE_ but maybe _NON_PURE_ would be
better?

The dev and user experience would certainly be better if we exploited
TypeScript types to ensure whatever is passed down to these methods is
marked as mutable (including `this`) and whatever comes out of the API
is immutable.

We could also expose these non-pure methods for special usage but with
extreme care. Maybe something like
`tree.mutable()._NON_PURE_push(x).immutable();` or
`tree.bulk((mutable) => mutable._NON_PURE_push(x));`. There are examples
on how to handle this in `funkia/list` and `immutable.js`. Is the
_NON_PURE_ prefix really necessary then?

The two previous points would allow and exploit the definition of a
`VolatileDeep` or `MutableDeep`, not sure on the name yet, whose right
and/or left digit is mutable ("or" because we could also settle on
`PushableDeep` and `ConsableDeep`). This would allow to replace digits
with either specialized mutable digit objects OR with JavaScript
Arrays depending on which solution is faster (member assignment + class
method table or Array push + switch on Array length).
This also has the benefit of reducing legacy bundle sizes.
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