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

feat(reporter): Replace memoizee library with a trivial solution. #1618

Merged
merged 1 commit into from
Oct 12, 2015

Conversation

DavidSouther
Copy link
Contributor

The reporter functionality was pulling the entire memoizee library with all its dependencies for a simple memoization solution, and using almost none of the functionality. This seems like not the most critically performant codepath. This PR replaces that with a good-case solution that lowers the dependency footprint.

@GitCop
Copy link

GitCop commented Oct 12, 2015

There were the following issues with your Pull Request

  • Commit: 0151c83
    • Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html


This message was auto-generated by https://gitcop.com

@DavidSouther DavidSouther changed the title feat(reporter) Replace way-too-big memoizee with a trivial solution. feat(reporter): Replace memoizee library with a trivial solution. Oct 12, 2015
@juliemr
Copy link
Contributor

juliemr commented Oct 12, 2015

Looks good to me. @zzo for final review.

@zzo
Copy link
Contributor

zzo commented Oct 12, 2015

please fix the (trivial) travis lint failure

The reporter functionality was pulling the entire memoizee library with all its
dependencies for a simple memoization solution, and using almost none of the
functionality. This seems like not the most critically performant codepath. This
PR replaces that with a good-case solution that lowers the dependency footprint.
@DavidSouther
Copy link
Contributor Author

please fix the (trivial) travis lint failure

Done.

zzo added a commit that referenced this pull request Oct 12, 2015
feat(reporter): Replace memoizee library with a trivial solution.
@zzo zzo merged commit 297eef6 into karma-runner:master Oct 12, 2015
@maksimr
Copy link
Contributor

maksimr commented Oct 12, 2015

@DavidSouther I dont think it's correct fix.
Property sourceMap may be object, see test. Current solution will return incorrect source map in this case.
Solution with weakMap allow cache object and clean cache when file with source map was removed.

\cc @dignifiedquire @zzo

@dignifiedquire
Copy link
Member

Agreed, there is a reason a proper cache was used here instead of this simple solution. Also the weakMap solution should be much more memory efficient than this. Unless there is a good reason please revert this.

@dignifiedquire
Copy link
Member

@DavidSouther I really don't mind a couple of files of code, but the question is the actual memory and performance footprint lowered by this? If you are concerned with file size you can implement a version using a weakMap instead, but please not using a simple object.

@zzo
Copy link
Contributor

zzo commented Oct 12, 2015

I believe the concern here is the dependency tree is large for this one module - I'll revert until we can find a better solution

@dignifiedquire
Copy link
Member

@zzo which I appreciate, but it shouldn't come at the cost of actual runtime behaviour and performance.

@zzo
Copy link
Contributor

zzo commented Oct 12, 2015

agreed thanks for weighing in

@DavidSouther DavidSouther deleted the feat/clean-deps/memoizee branch October 12, 2015 22:23
@DavidSouther
Copy link
Contributor Author

@zzo @dignifiedquire @maksimr Correct, I wanted this to remove the rather large memoizee dependency that's used in this one internal place.

I totally misread the tests, and thought only the string sourcemap contents were being used here. Because engines is still 0.10 || 0.12, WeakMap would be inappropriate. This seems like not the most performance critical code - it will be called once per error in testing output. What do you all think about using the string form of the sourcemap? sourceMap.file || sourceMap.contents || (typeof sourceMap == 'string' ? sourceMap : JSON.Stringify(sourceMap).

@dignifiedquire
Copy link
Member

We are already including a shim for weakMap, core-js (https://github.com/karma-runner/karma/blob/master/package.json#L233) so I suggest using that.

@DavidSouther
Copy link
Contributor Author

Ah, perfect! Should I re-open this PR, or start a new PR?

@dignifiedquire
Copy link
Member

Let's make it a new PR to have a clean slate (and I think Github gets confused otherwise)

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.

6 participants