Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

[Converge] mdb_v8 Related Commits #28

Closed
jasnell opened this issue May 22, 2015 · 31 comments
Closed

[Converge] mdb_v8 Related Commits #28

jasnell opened this issue May 22, 2015 · 31 comments

Comments

@jasnell
Copy link
Member

jasnell commented May 22, 2015

There are several mdb_v8 Related Commits

@bnoordhuis
Copy link
Member

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

@Fishrock123
Copy link
Contributor

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

@misterdjules
Copy link

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

@Fishrock123
Copy link
Contributor

@misterdjules was that ever upstreamed in the end?

@misterdjules
Copy link

@Fishrock123 It hasn't been upstreamed yet.

@jasnell
Copy link
Member Author

jasnell commented Jul 9, 2015

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

@misterdjules
Copy link

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

@Fishrock123
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

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.

@rvagg
Copy link
Member

rvagg commented Jul 29, 2015

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

@Fishrock123
Copy link
Contributor

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

@misterdjules
Copy link

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

Also, what is blocking exactly?

@Fishrock123
Copy link
Contributor

@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?

@davepacheco
Copy link
Contributor

@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.

@bnoordhuis
Copy link
Member

@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.

@davepacheco
Copy link
Contributor

@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.

@bnoordhuis
Copy link
Member

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.

@davepacheco
Copy link
Contributor

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.

@Fishrock123
Copy link
Contributor

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?

@davepacheco
Copy link
Contributor

@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.

@Raynos
Copy link
Contributor

Raynos commented Jul 29, 2015

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.

@davepacheco
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg commented Jul 29, 2015

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.

@davepacheco
Copy link
Contributor

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.

@rvagg
Copy link
Member

rvagg commented Jul 29, 2015

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

@Fishrock123
Copy link
Contributor

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?

@davepacheco
Copy link
Contributor

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

@misterdjules
Copy link

@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.

@rvagg
Copy link
Member

rvagg commented Aug 21, 2015

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.

@davepacheco
Copy link
Contributor

@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.

@rvagg
Copy link
Member

rvagg commented Aug 24, 2015

moved to nodejs/node#2517

@rvagg rvagg closed this as completed Aug 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants