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

benchmark: Fix hack by removing redundant + #17803

Closed

Conversation

sreepurnajasti
Copy link
Contributor

Fixes: nodejs/code-and-learn#72

/cc @AndreasMadsen

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels Dec 21, 2017
@Trott
Copy link
Member

Trott commented Dec 21, 2017

Might it be best to remove the comment but leave the + in the two lines of code? I don't think the + is the "hack" that the comment is referring to. It's in pretty much all our benchmarks and is fairly idiomatic JavaScript for "make this thing a number". While the code probably works fine without the +, leaving it out will make this different than our other benchmarks while leaving it in makes it clear to someone reading the code what the expected type of those two configuration settings is.

@BridgeAR
Copy link
Member

The "hack" was about actually having different internal n than the ones shown while running the benchmark.

@Trott
Copy link
Member

Trott commented Dec 21, 2017

The "hack" was about actually having different internal n than the ones shown while running the benchmark.

That makes a lot more sense. So perhaps the PR can be updated to try to fix that problem (which would be awesome 🎉) or updated to add more information to the comment so it's clear what it refers to (which would still be 👍).

@AndreasMadsen
Copy link
Member

@Trott The value n in common.createBenchmark is a number, thus it is also a number in main. If you write typeof conf.n it will show "number".

At some point, 1.5 years ago, this was not the case and the type would be a string. This is no longer the case, thus we can safely remove the +.

@AndreasMadsen
Copy link
Member

@Trott this is the code that makes it a number for all benchmarks: https://github.com/nodejs/node/blob/master/benchmark/common.js#L56

@sreepurnajasti
Copy link
Contributor Author

@BridgeAR Seems like the testcase running correctly without any use of internal n. Correct me if I am wrong.

@AndreasMadsen
Copy link
Member

@sreepurnajasti The benchmark tests are not very good, as they set all the values explicitly. Unfortunately, we can't afford to run the full benchmark in our tests.

AndreasMadsen
AndreasMadsen previously approved these changes Dec 21, 2017
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

Don't worry about the discusions. There is just some misunderstanding among us on how the benchmarks works, because they used to work differently.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 21, 2017

This comment (that I wrote, as I created this benchmark) was about n = 1e6 and not about anything else.

When you run the benchmark (e.g. node benchmark/run.js --filter deepequal-object.js assert) it will always show n set to this number, even though that is not true, since it will be reduced in the main part to have a similar running time for all size entries.

@AndreasMadsen it is clear that the number coercen is obsolete and that could be removed (when I wrote this benchmark I just copied the basic parts from other benchmarks and that was a common pattern, by now I already complain about this in newer benchmarks if someone does something similar) but this PR was about trying to fix the comment.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Dec 21, 2017

@BridgeAR Ah, I see.

My comment in nodejs/code-and-learn#72 was about the number coercen. As you said it is obsolete but it is still in a lot of benchmarks that people copy, thus it keeps showing up. That is why I thought it would be excellent for a code-and-learn.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Just to make sure this does not land as is.

@AndreasMadsen AndreasMadsen dismissed their stale review December 21, 2017 13:48

Misunderstod purpose of TODO

@Trott
Copy link
Member

Trott commented Dec 21, 2017

@AndreasMadsen @BridgeAR So it sounds like the TODO should stay but the + can be removed here and elsewhere? (If so, that's basically, the exact opposite of what I originally suggested, so sorry about that.)

@AndreasMadsen
Copy link
Member

@Trott completely agree.

@Trott
Copy link
Member

Trott commented Dec 21, 2017

(Oh, and maybe also: The TODO should be expanded to make it more clear what it is about.)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Dec 21, 2017

@BridgeAR Do you have some suggested text if the TODO is restored? TODO: n should correspond to conf.n and not conf.n / size? Maybe followed by one sentence explaining why the division is currently necessary?

@sreepurnajasti
Copy link
Contributor Author

@BridgeAR As per the discussion, I understood the following. Please, correct me if I misunderstood anything.

  1. Number coercion i.e removal of + from the code, which is done in the present pr.

  2. Todo:
    In the code, we have internal n (conf.n) , external n (conf.n/size). Is the fix here is to configure the internal n at runtime? If so, can it be by defining theconst n = 1e6; conf.n = n / size;in main functionality with few other appropriate changes?

@sreepurnajasti
Copy link
Contributor Author

@BridgeAR ping!!!

@AndreasMadsen
Copy link
Member

This is what I wanted:

-  const size = +conf.size;
+  const size = conf.size;
// TODO: Fix this "hack"
- const n = (+conf.n) / size;
+ const n = conf.n / size;

@sreepurnajasti
Copy link
Contributor Author

sreepurnajasti commented Jan 4, 2018

@AndreasMadsen Agreed. But, still the hack remains as is!! So can I even fix the hack in the same pr? Or should I make another for fixing the hack whereas this includes the above proposed changes?

@AndreasMadsen
Copy link
Member

Agreed. But, still the hack remains as is!! So can I even fix the hack in the same pr? Or should I make another for fixing the hack whereas this includes the above proposed changes?

Both options are acceptable. Do what you are most comfortable with.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2018

@sreepurnajasti the code change should look like the one @AndreasMadsen suggested.

I am not aware of a good fix for my comment at the moment, otherwise I would have done that already.
size and n would have to be set as a single option together. We could do something like a string that looks like "n/size".

@sreepurnajasti
Copy link
Contributor Author

@AndreasMadsen @BridgeAR Thanks. Done changes as per my understanding. Please review.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2018

@sreepurnajasti this does not fix the comment.

Right now, the best patch as far as I see it would be the one from @AndreasMadsen

-  const size = +conf.size;
+  const size = conf.size;
// TODO: Fix this "hack"
- const n = (+conf.n) / size;
+ const n = conf.n / size;

@sreepurnajasti
Copy link
Contributor Author

@BridgeAR Thanks. Its Done. Please review!!!

@@ -26,9 +26,9 @@ function createObj(source, add = '') {
}

function main(conf) {
const size = +conf.size;
const size = conf.size;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BridgeAR Can I remove this line and replace size with conf.size as it is only used in two places(31,34)?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to, yes. But I think it is not necessary.

@sreepurnajasti
Copy link
Contributor Author

I just want to make sure that no other changes are needed. If so, can someone run CI please.

@gireeshpunathil
Copy link
Member

@@ -26,9 +26,9 @@ function createObj(source, add = '') {
}

function main(conf) {
const size = +conf.size;
const size = conf.size;
Copy link
Member

Choose a reason for hiding this comment

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

If you want to, yes. But I think it is not necessary.

@BridgeAR
Copy link
Member

Landed in f6ae451

@BridgeAR BridgeAR closed this Jan 19, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 19, 2018
PR-URL: nodejs#17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member

@sreepurnajasti thanks for being patient! :-)

@sreepurnajasti
Copy link
Contributor Author

@BridgeAR @AndreasMadsen @Trott Thank you very much for assisting!!

@sreepurnajasti sreepurnajasti deleted the deepequal-object branch January 19, 2018 10:28
@AndreasMadsen
Copy link
Member

@sreepurnajasti Sorry, there was so much confusion. I'm glad it worked out :)

evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
PR-URL: #17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#17803
Refs: nodejs/code-and-learn#72
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code And Learn at NodeFest 2017
8 participants