[Converge] mdb_v8 Related Commits about node-convergence-archive HOT 31 CLOSED

Looks alright to me but the V8 changes in the last commit should be upstreamed first.

from node-convergence-archive.

Some extra discussion / context between myself and @misterdjules on irc about mdb: http://logs.libuv.org/io.js/2015-05-22#17:49:38.409

from node-convergence-archive.

FWIW, here's the issue that was used to track upstreaming the V8 changes in the last commit: nodejs/node-v0.x-archive#9147.

from node-convergence-archive.

@misterdjules was that ever upstreamed in the end?

from node-convergence-archive.

@Fishrock123 It hasn't been upstreamed yet.

from node-convergence-archive.

@misterdjules ... is there a timeline for getting those upstreamed?

from node-convergence-archive.

@jasnell There is no timeline. If it needs to be done quickly, please bump the priority in nodejs/node-v0.x-archive#9147.

from node-convergence-archive.

Please bump it. I think this is a converge blocker.

from node-convergence-archive.

Can we expedite this a bit? Is there anyone else who is familiar with mdb other than @misterdjules?

cc @nodejs/tsc If theres a point we'll fall behind for a converged release I think this is it.

from node-convergence-archive.

/cc @jclulow @davepacheco - halp? As @Fishrock123 points out above, this is a convergence blocker, can we have some Joyent eyes here?

from node-convergence-archive.

Also maybe cc @Raynos because I know he uses mdb a lot with node.

from node-convergence-archive.

I can work on upstreaming the V8 changes soon, but not today.

Also, what is blocking exactly?

from node-convergence-archive.

@misterdjules two things:

1. Any changes to v8 in io.js/converged must be upstreamed prior to floating them. (General policy)
2. Someone with knowledge of it has to actually update the biding for io.js/converged, I think?

from node-convergence-archive.

@Fishrock123: (1) makes sense. I don't understand "Someone with knowledge of it has to actually update the biding for io.js/converged, I think?".

I assume you meant "binding", and that you're referring to mdb_v8? So you're saying: given the V8 changes that have been pulled into the converged branch, the mdb_v8 module needs to be updated to work with Node versions from that branch? I think that's a fine goal, but it's an unrealistic constraint. Historically, we've updated mdb_v8 shortly before publishing a new major Node release (e.g., before 0.10, before 0.12), not lock-step with each V8 update in #master. If there are resources to keep mdb_v8 more up to date, that would be great.

This should not be confused with updating gen-postmortem-metadata.py so that the software continues to build. That's much less work than keeping mdb_v8 functional, and that does need to happen in lock-step, or V8 won't build.

For reference, I have moved the canonical copy of mdb_v8 to https://github.com/joyent/mdb_v8 to decouple it from Node. I think we should assume moving forward that that project will be updated to support newer Node releases as soon as reasonable, but with some lag behind the V8 updates.

from node-convergence-archive.

@davepacheco The expectation is that someone, presumably a Joyent employee, keeps the postmortem support in V8 in working shape.

It's fine if that's not possible due to resource constraints but it means we'll disable postmortem support by default, i.e., people will have to opt-in at configure time (which may not work if it's not up to date) and it won't be enabled in release builds.

from node-convergence-archive.

@bnoordhuis As I mentioned in my previous comment, there are two pieces of the postmortem support. The V8 piece is just gen-postmortem-metadata.py. We're happy to make sure that gets kept up to date (though I believe people outside of Joyent have generally been doing that, often without us realizing it, which has been okay). The second piece is mdb_v8, which is not part of V8, does not generally break at build-time when V8 changes, and does not need to be kept up to date.

It's very important that we continue to build with the V8 postmortem debugging support even if the mdb_v8 updates are delayed. That way, when mdb_v8 updates are made later, they will work with all previous versions that have the metadata. If that metadata is disabled, then those Node releases will be undebuggable using these tools forever.

from node-convergence-archive.

We're happy to make sure that gets kept up to date (though I believe people outside of Joyent have generally been doing that, often without us realizing it, which has been okay).

That probably was Fedor and me, around V8 upgrades. :-) I drew the line a while ago after the umpteenth broken upgrade. If you can keep the postmortem support in V8 up to date, I think we're good.

from node-convergence-archive.

That sounds great. (And thanks for taking care of it in the past. I know it can be painful.) If someone pings myself, @bcantrill, or @misterdjules when gen-postmortem-metadata breaks, we'll be happy to deal with it.

from node-convergence-archive.

So do we not actually need this stuff in core then? Should we just point to the external project you have? Isn't that a pretty big breaking change for users, or?

from node-convergence-archive.

@Fishrock123 That's a good question. One of the reasons we put this into Node core in the first place is because we build mdb_v8 into the Node binary so that the debugger can use that copy, so that users don't need a separate binary. Support for this is not complete in our debugger, though. The ideal situation for users of mdb_v8 is for Node to keep taking changes from upstream mdb_v8 and keep building it into the binary the way we're doing now.

There's also a postmortem debugging working group which has talked about elevating the role of postmortem debugging. It would be nice if this were first-class in Node, documented, and the tools were available there, but that's up to the working group to talk about.

If there's a compelling reason to remove it, that would be reasonable, but all things being equal I'd prefer to keep it.

from node-convergence-archive.

On a similar note; What are we going to do about keeping src/v8ustack.d upto date ?

I've used both dtrace and mdb for production debugging purposes; I'm actually more excited about having working dtrace (and our stap port) support iojs.

from node-convergence-archive.

I generally fix v8ustack.d whenever anyone tells me it's broken.

from node-convergence-archive.

Is it safe to recommend to collaborators to ping @nodejs/post-mortem to get the attention of someone to fix broken mdb functionality or should we add a new team for specific people? We have these already for things like platform-windows, platform-solaris, etc., we could add a mdb team with specific people if it shouldn't just be post-mortem.

from node-convergence-archive.

For now, I would add a separate team for this. The postmortem group has not decided exactly what it's going to cover, and undoubtedly there will be members there that care primarily about other tools and don't care about mdb_v8. I would recommend putting myself, @bcantrill, and @misterdjules on an "mdb/dtrace" team.

from node-convergence-archive.

@nodejs/dtrace-mdb created and folks added/invited

from node-convergence-archive.

Just a reminder, that was two weeks ago.

So what I understand is that we really only need to re-enable post-mortem debugging for v8? Is that correct?

from node-convergence-archive.

Was postmortem debugging disabled? I thought the only work necessary here was due to policy, and that's to upstream a small V8 change.

from node-convergence-archive.

@davepacheco @jasnell Created https://codereview.chromium.org/1296743003 to upstream one part of the V8 change.

I don't think we want to upstream the other part of the change which adds fieldindex_mask and fieldindex_shift constants, since they duplicate existing constants with the same value. I just need to figure out if that will break older mdb_v8 versions loading newer V8 core dumps and whether this is a significant use case.

from node-convergence-archive.

Sadly I don't seem to have brain space to grok everything in this issue but I'm wondering if we can narrow it all down to a simple question: Will we be able to say that mdb supports Node.js v4 out of the box? The release is planned for the end of this month (maybe beginning of next month) so it's coming up soon and the mdb component is an important part of the "please upgrade" story that we're all pushing so we can help users migrate off 0.10 and 0.12 to v4 without too much pain.

The version of V8 in io.js v3.x (nodejs/node/v3.x) and in nodejs/node/master (v4.4) is very likely to be what ships with v4. It seems very unlikely that Chrome 45 will ship before Node.js v4 does and even if it did, the timeframe for landing and testing V8 v4.5 will be too short to justify the scramble. So I'd say it's safe to consider the master branch a safe bet for what V8 is going to look like in v4 and if you need to float any patches then now would be the time to do it.

An alternative, that is not as ideal, is to punt on this for the initial release but get anything done during the period between v4 being released and when it turns into an LTS, at which point we won't be able to land any feature additions. LTS is planned for the end of October so there's a couple of months within which to add feature additions (semver-minor) if necessary. This is not ideal IMO because it doesn't help the rallying cry to get people moved over to v4, it'll defer even an investigation phase for many large users of Node that care about mdb support.

from node-convergence-archive.

@rvagg I'm afraid there's no getting around the complexity, but it's not that complex: the Node binary has built-in metadata that mdb uses, but the mdb functionality is not in Node itself. What's most critical is that the metadata be present, complete, and correct. Then the functionality can come later without having to change Node.

If we also want to make sure the debugger functionality works when Node 4 is released, that's great. Someone needs to test (and potentially fix) that. I'm not sure I can prioritize that in the next week or two.

from node-convergence-archive.

moved to nodejs/node#2517

from node-convergence-archive.