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

Format of v8 log file changed in v15 #22

Open
pbadenski opened this issue Nov 26, 2020 · 7 comments
Open

Format of v8 log file changed in v15 #22

pbadenski opened this issue Nov 26, 2020 · 7 comments

Comments

@pbadenski
Copy link
Collaborator

So far I noticed this for IC info - row, column and map fields are not read property. I don't mind submitting pull request, but need a bit of guidance:

  • is there a place where I can look this format up - there seems to be a new field there and I don't know what it is
  • is there a way to specify different parser and separate code paths for log files dumped by node v15
@thlorenz
Copy link
Owner

thlorenz commented Jan 8, 2021

I'm sorry, but I don't have much bandwidth to help here, however of course I'd welcome a PR that doesn't break older versions + makes it work for newer ones.
I have no place to look up the format, but maybe you could verify if the upgrade to v8-tools-core fixes the issue?

Thanks

@pbadenski
Copy link
Collaborator Author

pbadenski commented Jan 8, 2021

Cool, I'll check out. I've actually learnt more about this since my message, so feel relatively comfortable submitting a PR.

@pbadenski
Copy link
Collaborator Author

Looks this is a bit tricky and requires dealing with backwards compatibility. One solution is to introduce separate (and largely duplicated) copy of DeopyProcessor for v15.

A separate question I have - why DeoptProcessor is pasted directly into deoptigate and packaged via v8-tools-core? As far as can I understand functionality of DeoptProcessor is very similar to v8-tools Processor https://github.com/v8/v8/blob/master/tools/system-analyzer/processor.mjs. I'm asking, because this versioning could also be provided (and centralised) in v8-tools-core so you could have v8-tools-core/v14, v8-tools-core/v15, etc.

@pbadenski
Copy link
Collaborator Author

@thlorenz thoughts?

@thlorenz
Copy link
Owner

thlorenz commented Jun 14, 2021

The v8-tools-core package is code I pulled from inside the v8 repo in order to use it for purposes like deoptigate. It is a dependency that we can upgrade when needed.

If the v8 code that it pulled changed, then we should update that module and bump the version here.

why DeoptProcessor is pasted directly into deoptigate

If a package that does the same is available or could be created similar to how I did with v8-tools-core, then we should do that. And I'm totally for a way to version this, that was my intent when I created v8-tools-core.
It could be that modules doing the same have propped up in the meantime and we should use them if possible.

this is a bit tricky and requires dealing with backwards compatibility

IMHO how we should handle this is to do all the above and use those modules to then create a version of deoptigate that is compatible with the newer v8 and its output format.
Then we should just upgrade the major version and release + add a note in the README for people that may want to install an older version of deoptigate.
I don't see a problem with that, esp. given that deoptigate is mostly used as a command line tool.

I hope that answers all your questions and helps us get on the right track with this.

One final note, I've been toying a while with the idea of rewriting deoptigate to TypeScript, just had no bandwidth. To be honest I'm not too happy with some of the code (it's not the cleanest). Therefore if the upgrade you're proposing would involve a lot of changes we should think about rewriting in TS as part of those.

@pbadenski
Copy link
Collaborator Author

Great to hear this!

  1. I think it's best to move DeoptProcessor work to a separate issue - I will do that.
  2. In terms of backwards compatibility I was thinking it could be nice to keep one deoptigate code for all versions of node. We could pull different version of v8-tools-core depending on the context. Isolate logs include both v8 and node version in the first line. We could read that and pull v8-tools-core-v14, v8-tools-core-v15, etc. with different parser implementation dynamically. Less hassle for the user and shouldn't be that much more complex to manage this.
  3. I'm not yet sure exactly about the scope, but I do like TypeScript :)

@thlorenz
Copy link
Owner

Awesome .. I agree with all those points .. the 3rd we can punt on, I just let you know to avoid you putting too much work into reimplementing something that is planned to be moved to TS eventually.

LMK how I can help (even though my bandwidth is somewhat limited).
Looking forward to the new modules + PRs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants