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

Filter by types, categories and properties seperately #39

Merged
merged 22 commits into from
Nov 29, 2018

Conversation

AlanSl
Copy link
Contributor

@AlanSl AlanSl commented Nov 20, 2018

This mostly changes things under the hood:

  • Removes the typeTEMP property (long overdue!)
  • Uses isNodeExcluded() from shared.js to test for exclusion everywhere - in analysis, in the UI, and pipes it in as an override for the d3-fg test that sets .hide on frames
  • Therefore, exclusion tests now check for properties like isInit separately to categories like app, deps, etc, and separately to types like v8, cpp; and for deps, the types are now the name of the dependency
  • Namespaces type-based exclusion by category. So, for example, if someone uses the module native as a dependency, there's no ambiguity between deps:native and all-v8:native exclusion filters

This means:

  • (the only visible change expected in this PR) It should be more accurate with init frames. It no longer treats init as a type, which means that if an init frame is not from core, it will now tell you so and treat it more appropriately. For example, here's a frame that is init and also from a dependency:

0x init

  • More importantly, this puts us in a position where we're very nearly ready to add features based on additional types, such as checkboxes to filter out specific dependencies. We just need analysis to generate the list, which is WIP by @goto-bus-stop

This touches on a lot of code so will need thorough testing for regressions.

@AlanSl
Copy link
Contributor Author

AlanSl commented Nov 22, 2018

Still needs tests, now works again with the new changes to master, particularly the new core / V8 categorisation. Here's another example of a rare init-but-not-core frame:

image

@AlanSl AlanSl changed the title [WIP] Filter by types, categories and properties seperately Filter by types, categories and properties seperately Nov 22, 2018
@AlanSl
Copy link
Contributor Author

AlanSl commented Nov 22, 2018

Here's one regression:

image

I'll add a commit setting the type for app frames to the app name so such things would be like In profiled application (some-amazing-app)

@AlanSl
Copy link
Contributor Author

AlanSl commented Nov 22, 2018

Last few commits tidy up the presentation of the code area text in the info box:

image

image

image

image

image

image

Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good so far (± one structural comment), will play around with it a bit more

analysis/frame-node.js Outdated Show resolved Hide resolved
analysis/frame-node.js Outdated Show resolved Hide resolved
visualizer/data-tree.js Show resolved Hide resolved
@goto-bus-stop
Copy link
Contributor

there's a slightly strange colour variance here:

image

v8/v8 native have the grey colour, while node/v8 runtime have the greyish greenish one

Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending ci/rebase

@cuginoAle
Copy link
Contributor

Late to the party...
LGTM!

@AlanSl AlanSl mentioned this pull request Nov 29, 2018
@AlanSl
Copy link
Contributor Author

AlanSl commented Nov 29, 2018

With #53 merged I can rebase away the last few commits working around the 0x update breaking the tests.

visualizer/ui.js Outdated
const nodeInvalidMessage = ' node selected in selectHottestNode'

// Prevent infinite loop if some future bug allows an invalid node to be returned here
if (!node) throw new Error ('No' + nodeInvalidMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little heads-up: You might have a Lint error here because of the space between Error and ('

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely, the linter doesn't mind, but it probably should mind, so I've changed it anyway 🙃

@cuginoAle
Copy link
Contributor

It looks good to me.
One thing that would be relevant for this as well : we currently throw and error if no node is selected, that means we shouldn't be in a scenario where no data is displayed (either because the user filtered everything out or simply because 0x didn't collect any data).
Maybe we could notify the user that there's no data available showing a Message rather then throwing an error?

@AlanSl
Copy link
Contributor Author

AlanSl commented Nov 29, 2018

The difficulty with that issue is that there are lots of points in the process that assume that data exists and it's difficult to stop them all from hitting errors. Most components expect something on each update.

That comment has however given me an idea for a different way to solve the problem... and it seems to work. Will post a PR shortly, after merging this one.

@AlanSl AlanSl merged commit e186fcc into master Nov 29, 2018
@goto-bus-stop goto-bus-stop deleted the types-categories branch March 19, 2019 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants