deps: backport 9da3ab6 from V8 upstream

[ofrobots]

This patch exposes a new flag perf_basic_prof_only_functions (disabled by
default) that can be useful for the use-case of running always-on profiling on
long running production jobs.

This is already present on master, but would be good to get back-ported to v4.x.
R=@bnoordhuis, @nodejs/v8

Original commit
v8/v8@9da3ab6
New flag --perf_basic_prof_only_functions
Restricts linux perf-event code range reporting to functions only (i.e. on
stubs.) While this makes the gathered ticks less accurate, it reduces the
growth of the /tmp/perf-${pid}.map file.

BUG=v8:3453
R=[email protected],[email protected]
LOG=N

Review URL: https://codereview.chromium.org/1292743002

Cr-Commit-Position: refs/heads/master@{#30179}

[indutny]

This seems to be a feature addition, adding semver-minor

[targos]

cc @nodejs/lts

[jasnell]

As a semver-minor it cannot land in v4.x.

[ofrobots]

@jasnell Is your statement about semver-majors? Are we never going to have a node 4.3.0??

This is bug fix that is being fixed by a new off-by-default flag. This is low risk. I'm fine with this waiting for 4.3, but can you provide more justification why it cannot go into v4 at all?

[yunong]

This is an extremely important, yet low risk feature for most enterprise customers. This effectively means that we're able to perform production CPU profiling at any time. It seems silly to have to wait basically until October 2016 for 6.0 to land for this feature to show up in an LTS supported release.

[jasnell]

I get that but similar arguments could be made for any semver-minor commit
-- they could all be useful for developers using the LTS but that alone
doesn't justify landing it. The current LTS policy says no semver-minor or
semver-major changes unless it's part of a security fix. The reason for
that is that semver-minor changes can actually be quite disruptive.

That said, the LTS WG and CTC can look at individual commits to see if it's
worth making a case-by-case exception. The general policy isn't always
ideal and this can be put on the LTS WG meeting agenda and reviewed. /cc
@nodejs/lts @nodejs/ctc

[jasnell]

[bnoordhuis]

I agree with @jasnell. Landing this in the LTS branch would set a bad precedent.

[jbergstroem]

Saying No to merging here is the right thing to do and important moving forward so we don't end up on a slippery slope. I think the comment (ok, both comments) from @jasnell perfectly explains this.

[ofrobots]

bad precedent
slippery slope

I propose we evaluate this PR on merit instead:

1. Does this fix a real problem?
2. Does it introduce an unacceptable level of risk?

I haven't seen an argument against this PR on merit yet.

Remember that LTS is not about 'no changes'. It is about managing risk while still fixing real problems that people have.

[jasnell]

I've got no problem at all with evaluating this PR we just need to be
certain what the approach is going to be. We've been saying all along that
there will be no semver-minor or higher in LTS and now that we're two weeks
into our first LTS we're considering landing a semver-minor. What's the
world coming to!? OK, so it's not really that big of a deal, we just need
to keep things consistent. If we're going to land some semver-minors,
which are we landing and which are we not? Which PRs have more merit than
others?
On Nov 1, 2015 6:25 PM, "Ali Ijaz Sheikh" [email protected] wrote:

bad precedent
slippery slope

I propose we evaluate this PR on merit instead:

1. Does this fix a real problem?
2. Does it introduce an unacceptable level of risk?

I haven't seen an argument against this PR on merit yet.

Remember that LTS is not about 'no changes'. It is about managing risk
while still fixing real problems that people have.

—
Reply to this email directly or view it on GitHub
#3609 (comment).

[ofrobots]

Which PRs have more merit than others?

Why not evaluate each PR on its own merits? Why compare them to other PRs?

[ofrobots]

saying all along that there will be no semver-minor or higher in LTS

It seems like you're focussing on syntax instead of the semantics.

[jasnell]

Not so much. There's no particular rush on getting this into v4 is there? I
put the issue on the LTS WG agenda to discuss, let's allow that discussion
to happen.
On Nov 1, 2015 7:15 PM, "Ali Ijaz Sheikh" [email protected] wrote:

saying all along that there will be no semver-minor or higher in LTS

It seems like you're focussing on syntax instead of the semantics.

—
Reply to this email directly or view it on GitHub
#3609 (comment).

[yunong]

@jasnell I can't overstate this enough, but this is a huge deal for us here at Netflix, and I dare say for other large enterprise customers as well. I recall that there were engineers from Uber and Peer5 on the original v8 issue (https://code.google.com/p/v8/issues/detail?id=3453) that also wanted to see this fixed in v8. Maybe @mranney can chime in here with Uber's perspective.

Not sure how many folks here are running large Node production deployments in mission critical paths, but without a way to quickly and easily observe and profile Node application performance in production on Linux means that this is literally stifling Node adoption both internally at Netflix and probably externally as well. Technically, this may be a new feature -- but it is really a v8 bug fix. The v8 team has done a great job getting this bug fixed, and has done so for some time. Indeed I seem to recall that this patch landed in v8 (https://chromium.googlesource.com/v8/v8.git/+/9da3ab661fe7190fcb99bd99db30cf95913d3659) before node v4 was launched.

Will we have to wait ~10 months before we're able to get this feature in an LTS release? That's 10 months of not having performance visibility into our critical Node applications which serve ~70 million subscribers.

I'm happy to chat more on Monday at the WG meeting -- but I thought I'd share Netflix's perspective ahead of it.

[jbergstroem]

@yunong have you considered floating it on top of a LTS release if its that significant? Just saying that your agenda might be different than the LTS agenda in more than one scenario.

[yunong]

Just reading the v4 release notes, it seems that from
Node.js v4 also brings a plan for long-term support (LTS) and a regular release cycle. Release versioning now follows SemVer, so expect increments of both minor and patch version over the coming weeks as we fix bugs and add features. What we won't be doing within the v4.x branch is adding breaking changes.
that this change should make it in? It is a bug fix and a non-breaking changing, isn't it?

[jasnell]

I get that this is important but one LGTM from me is not enough and there's
no reason that I can see to rush the discussion or the decision. v4.2.2 is
pretty well locked down and it's likely to be at least a week or more
before the next round after that. As I said, it's on the LTS WG agenda so
it will be discussed and worked out.
On Nov 1, 2015 7:42 PM, "Yunong Xiao" [email protected] wrote:

@jasnell https://github.com/jasnell I can't overstate this enough, but
this is a huge deal for us here at Netflix, and I dare say for many
large enterprise customers of Netflix as well. I recall that there were
engineers from Uber and Peer5 on the original v8 issue (
https://code.google.com/p/v8/issues/detail?id=3453) that also wanted to
see this fixed in v8. Maybe @mranney https://github.com/mranney can
chime in here with Uber's perspective.

Not sure how many folks here are running large Node production deployments
in mission critical paths, but without a way to quickly and easily increase
Node application performance in production on Linux means that this is
literally stifling Node adoption both internally at Netflix and probably
externally as well. Technically, this may be a new feature -- but it is
really a v8 bug fix. The v8 team has done a great job getting this bug
fixed, and has done so for some time. Indeed I seem to recall that this
patch landed in v8 (
https://chromium.googlesource.com/v8/v8.git/+/9da3ab661fe7190fcb99bd99db30cf95913d3659)
before node v4 was launched.

Will we have to wait ~10 months before we're able to get this feature in
an LTS release? That's 10 months of not having performance visibility into
our critical Node applications which serve ~70 million subscribers.

I'm happy to chat more on Monday at the WG meeting -- but I thought I'd
share Netflix's perspective ahead of it.

—
Reply to this email directly or view it on GitHub
#3609 (comment).

[trevnorris]

Should we consider the addition of a flag of a dependency a minor bump? Personally don't feel like this qualifies as a minor in the usual sense, and am fine landing it on LTS.

LGTM

[rvagg]

The main point of sticking to the patch-level changes is that the risk is decreased. Adding features necessarily involves making the kind of changes that can lead to bugs and regressions. This kind of assessment has to be applied to patch-level changes too—the more they mess with internals the more risk they carry and the less they qualify for LTS. A flag for enabling certain things that would otherwise not make it in doesn't help any of this.

[trevnorris]

@rvagg

A flag for enabling certain things that would otherwise not make it in doesn't help any of this.

Except this flag inhibits functionality. The code change is very straightforward, as it only acts like a filter for output of a existing flag. Another reason why I consider this more a patch level change.

[mranney]

This is certainly a very useful thing, but I don't think it's important enough to enough users to warrant deviating from the release schedule. This is sad, but IMO is a reasonable consequence of a predictable release schedule and process.

At Uber we don't depend on perf at all, and we are also not on node 4. I think big operations that actually engage in performance profiling will have enough sophistication to float this patch, and they probably already have a custom build process for their version of node anyway.

[rvagg]

Flagging this for ctc-agenda for @nodejs/ctc discussion because the LTS WG would like to discuss the semver impact of this change.

We've agreed to land this in v4.x and know that the messaging is going to be really tough re versioning ("hey, this is supposed to be stable, why are you adding features?", "I thought you said it'll likely be 4.2.x for most of its life but now it's 4.3??") and also the fact that we've decided to deal with these on a case-by-case basis and there will be other items come up that people will want in LTS ("but you took #3609!").

We've had a number of discussions in the past about how our dependencies impact semver but we don't have a strict policy that would help us decide on this item. It's difficult to make a case that this is not a semver for V8 and it impacts on our CLI options because we expose V8 CLI options, is that enough to force semver-minor or can we argue this is a semver-patch because it's not a Node.js feature addition?

[yunong]

@jasnell Thanks for the update and all the hard work 👍

[MylesBorins]

landed in v4.x-staging as 86899c1