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

check: return instead of throw internally #4584

Closed
wants to merge 3 commits into from
Closed

Conversation

@wh0
Copy link

@wh0 wh0 commented Jun 17, 2015

Profiling our teams app showed that a lot of time was spent creating errors for the check package, which I assume creates a stack trace each time. For every Match.Optional or Match.OneOf, it would have to create a stack trace for the one choice that failed.

Anyway, this PR modifies check's internal implementation to use more return instead of throw, so as not to construct so many error objects. It shouldn't change the external API of check or Match.Where.

This sped up one of our app's benchmarks, which called Collection::find many times (find internally uses check to validate its second parameter).

@@ -145,33 +139,41 @@ var typeofChecks = [
[undefined, "undefined"]
];

var checkSubtree = function (value, pattern) {
// Return `false` if it matches. Otherwise, return an object with a `message` and a `path` field.
var testSubtree = function (value, pattern) {

This comment has been minimized.

@wh0

wh0 Jun 17, 2015
Author

This doesn't correspond to test the way checkSubtree corresponded to check.

@apollo-cla
Copy link

@apollo-cla apollo-cla commented Jun 17, 2015

@wh0: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@jperl
Copy link
Contributor

@jperl jperl commented Jun 19, 2015

❤️

if (value === null)
throw new Match.Error("Expected object, got null");
return {

This comment has been minimized.

@stubailo

stubailo Jun 19, 2015
Contributor

Since these if statements have become multiple lines, let's wrap the body in brackets.

_.each(requiredPatterns, function (subPattern, key) {
throw new Match.Error("Missing key '" + key + "'");
});
for (var keys = _.keys(requiredPatterns), i = 0, length = keys.length; i < length; i++) {

This comment has been minimized.

@stubailo

stubailo Jun 19, 2015
Contributor

This doesn't actually need to be a loop, right? It will return on the first iteration, so we may as well do that in the code explicitly (I know the old code had a loop, but we should fix this now that we have the chance)

This comment has been minimized.

@wh0

wh0 Jun 19, 2015
Author

oh yeah, you're right. The conversion was so mechanical that I didn't even notice.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Jun 23, 2015

@wh0 still waiting on the for loop change! Excited to get this change in.

@stubailo stubailo self-assigned this Jun 29, 2015
@mitar
Copy link
Collaborator

@mitar mitar commented Jun 29, 2015

Yea!

@wh0 wh0 closed this Sep 17, 2015
@wh0 wh0 deleted the wh0:check-nothrow branch Sep 17, 2015
@mitar
Copy link
Collaborator

@mitar mitar commented Sep 17, 2015

Why closing?

@wh0
Copy link
Author

@wh0 wh0 commented Sep 17, 2015

whoa, I accidentally deleted my branch

@wh0 wh0 restored the wh0:check-nothrow branch Sep 17, 2015
@wh0 wh0 reopened this Sep 17, 2015
@stubailo
Copy link
Contributor

@stubailo stubailo commented Sep 17, 2015

Lol, I should merge this

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 17, 2015

Yes you should. :-)

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 13, 2015

Merged: 4f79c60

@wh0, you should set up an email address in Git so that GitHub can track your commits! Otherwise it will be super hard to trace them back to you if something comes up like a question.

@stubailo stubailo closed this Oct 13, 2015
@wh0 wh0 deleted the wh0:check-nothrow branch Oct 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.