Node: Explicit conditions consideration

[vsemozhetbyt]

• Version: maybe all
• Platform: maybe all
• Subsystem: v8

It seems these remarks (mostly the second part of the article) can be a starting point for some future code-and-learn sessions and good first contributions — concerning tests, benchmarks, and even libs:

http://benediktmeurer.de/2017/04/03/v8-behind-the-scenes-march-edition/

[mscdex]

Some of the things mentioned may be good general tips, but some may not be (for node). For example the suggestion about using default params would not typically work well for node because we often have optional parameters for many APIs and there would be no way for V8 to know if a "middle option" was not supplied, so we will still have to do some duck typing and such to figure things out.

As far as the optimization suggestions go, we should be very clear in such code-and-learn sessions that they are only applicable for very recent (as of this writing) versions of V8.

[fhinkel]

I'd be very hesitant to suggest optimization work for code-and learn.

[gibfahn]

I'd be very hesitant to suggest optimization work for code-and learn.

I think it might be good for a second contribution (i.e. for people who've done something simple, and are looking for something a little more involved), which we do get quite a few of.

However I think we'd want to already know what changes we wanted to recommend, as in as specific "change A to B in this file", rather than "this is a good optimisation tip, have a go".

[joyeecheung]

Getting optimization PRs in usually involves running benchmarks, which means people would probably need to go through https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md first in addition to going through https://github.com/nodejs/node/blob/master/BUILDING.md and https://github.com/nodejs/node/blob/master/CONTRIBUTING.md (and they would probably need to deal with installation of the analysis tools too), so uh...I don't think it's sutiable for code-and-learn either ;) But yeah those can be good second contributions.

[joyeecheung]

(Although ^ can be alleviated if we have CI jobs for benchmarks, still, for some benchmarks people would need to wait for a while before the results come out..eh.. :/

[vsemozhetbyt]

@joyeecheung I mostly meant changes in benchmarks and tests for the begginers, but on the other hand those files maybe are not worth to bother about performance.

[joyeecheung]

@vsemozhetbyt Oh, sorry, missed that part in the OP..yeah, I think some of the advices can be good candidates for code-and-learn ssesions (like replacing if (obj) with if (obj !== undefined) to avoid subtle bugs, probably in tests where we don't need to worry about regressions)

[Trott]

I'm not sure these are going to be that easy to find, even in tests, unless we're OK changing stuff like:

if (common.isWindows) {
  // do something Windows-specific
}

…to this:

if (common.isWindows === true) {
  // do something Windows-specific
}

I wouldn't want to give that to a first-time contributor unless I was certain they weren't going to be asked to defend the change in PRs. I like to give first-time folks uncontroversial changes as much as possible.

[vsemozhetbyt]

@Trott So maybe we can close this issue for now?

[Trott]

@Trott So maybe we can close this issue for now?

Given that it was (until today) idle for four months, that's probably a good idea. If anyone disagrees, of course, feel free to re-open...