Collapse functions subtree transform #721

gregtatum · 2018-01-18T19:48:50Z

Please only review the last 3 commits, this is based off of the flow upgrade PR #709. I will rebase before merging.

9e7af51 Make transforms exhaustively checked
a36ed92 Add memoize-immutable type definition
bce2123 Collapse function subtree transform; resolves #545

codecov · 2018-01-18T19:54:38Z

Codecov Report

Merging #721 into master will increase coverage by 0.39%.
The diff coverage is 71.01%.

@@            Coverage Diff            @@
##           master    #721      +/-   ##
=========================================
+ Coverage    62.5%   62.9%   +0.39%     
=========================================
  Files         121     121              
  Lines        7220    7313      +93     
  Branches     1633    1658      +25     
=========================================
+ Hits         4513    4600      +87     
- Misses       2378    2383       +5     
- Partials      329     330       +1

Impacted Files	Coverage Δ
.../components/calltree/ProfileCallTreeContextMenu.js	`0% <0%> (ø)`	⬆️
src/test/fixtures/utils.js	`100% <100%> (ø)`	⬆️
src/url-handling.js	`65.03% <50%> (-1.42%)`	⬇️
src/utils/flow.js	`71.42% <64.28%> (+56.04%)`	⬆️
src/reducers/profile-view.js	`83.43% <75%> (+0.1%)`	⬆️
src/profile-logic/transforms.js	`87.01% <81.31%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde6801...ec85fb7. Read the comment docs.

julienw

I think there's enough nits and comments to warrant a new review :)

julienw · 2018-01-19T16:19:52Z

src/types/transforms.js

+   *                        ↓                               ↓
+   *                      A:1,0                           X:1,1
+   */
+  'focus-subtree': {|


you never use the keys in this type. Moreover they're duplicating the types. So why don't you just use an array instead of an object ? This would be Transform already.

Then you'd simply use $TupleMap<...> instead of $Values<$ObjMap<...>>.

As far as I know I can't convert a tuple into a union. The $TupleMap returns another tuple. See here in flow.org/try.

I referenced a Flow issue in the comment above.

ah it's so sad we can't get an union from an array's values... Thanks for trying.

julienw · 2018-01-19T17:00:59Z

src/profile-logic/transforms.js

-        // Do not throw an error, as we don't trust the data coming from a user.
-        console.error('Unrecognized transform was passed to the URL.', type);
-        break;
+        throw assertExhaustiveCheck(type);


the function itself is throwing already, why calling "throw" here ?

same below of course

Flow continues to go through the code, as it doesn't realize that assertExhaustiveCheck throws inside of it. Some places I can take out the extraneous throw without Flow complaining, but sometimes it is needed. I checked and this one is not needed, but tried to remove others and Flow complained. My preference would be to throw an additional time.

I see; and after all this makes it quite clear when reading that we can throw here.

julienw · 2018-01-22T13:14:14Z

flow-typed/npm-custom/memoize-immutable_v3.x.x.js

+// @flow
+
+declare module 'memoize-immutable' {
+  declare module.exports: <T: Function>(fn: T, config: Object) => T;


config is optional and has 2 mutually exclusive parameters.

I took some time to do a libdef for flow-typed but it's not fully complete and I don't want to take the time to make it work...

So for now, maybe something like this would be good enough:

interface CacheInstance<K, V> { has(K): boolean; get(K): V; set(K, V): void; }; type CacheConfig<K, V> = {| cache: CacheInstance<K, V>, |}; type LimitConfig = {| limit: number, |}; declare module.exports: <F: Function, K, V>(fn: F, config ?: CacheConfig<K, V> | LimitConfig) => F;

And this will expose that this is incorrect: https://github.com/devtools-html/perf.html/blob/e4b3388a1f3dc742a925804b18a1041e28f1a372/src/reducers/profile-view.js#L561-L564

Indeed memoize-immutable passes "limit" only if "cache" isn't specified: https://github.com/memoize-immutable/memoize-immutable/blob/f4447e041ce7956455c9276191f5314d2bea08ea/index.js#L11

But anyway WeakTupleMap doesn't handle a "limit" as parameter: https://github.com/memoize-immutable/WeakTupleMap/blob/5a794788b942ac148d30cbfc0e2b8b313a0b3f35/index.js#L1-L3

julienw · 2018-01-22T14:00:03Z

src/components/calltree/ProfileCallTreeContextMenu.js

+          data={{ type: 'collapse-function-subtree' }}
+        >
+          <span className="profileCallTreeContextMenuIcon profileCallTreeContextMenuIconCollapse" />
+          {"Collapse function's subtree"}


nit: I believe you used {" ... "} because of the eslint rule, but the real fix is rather to use the nice quote ’ :) (shamelessly copied from wikipedia: https://en.wikipedia.org/wiki/Apostrophe)
=> Collapse function’s subtree

Actually I think we need to make it clearer that this happens accross the whole tree => Collapse this function’s subnodes accross the tree
I changed "subtree" so that I can use the word "tree" without a repetition. You could also use "descendants" but I wasn't sure how accurate this term would be.

I have no way to type the nice quote directly and have never used it in my English writing, not sure why we should make it more complicated than it needs to be.

I updated the label to be more explicit.

julienw · 2018-01-22T14:35:29Z

src/profile-logic/transforms.js

+                type: 'collapse-function-subtree',
+                funcIndex,
+              });
+              break;


nit: please use the same order for the case blocks here and the case lines above.

julienw · 2018-01-22T15:36:57Z

src/profile-logic/transforms.js

+  'collapse-function-subtree',
+].forEach((transform: TransformType) => {
+  // This is kind of an awkward switch, but it ensures we've exhaustively checked that
+  // we have a mapping for every transform.


Are you sure this works as you think? I believe you can add or remove something from the array you're iterating with without flow warning you...

This seems to work better

type TransformTypes = // comes from types/transforms.js | 'focus-subtree' | 'focus-function' | 'merge-call-node' | 'merge-function' | 'drop-function' | 'collapse-resource' | 'collapse-direct-recursion'; const TRANSFORM_TO_SHORT_KEY: {[key: TransformTypes]: $Keys<typeof SHORT_KEY_TO_TRANSFORM>} = { 'focus-subtree': 'f', 'focus-function': 'ff', 'merge-call-node': 'mcn', 'merge-function': 'mf', 'drop-function': 'df', 'collapse-resource': 'cr', 'collapse-direct-recursion': 'rec', }; const SHORT_KEY_TO_TRANSFORM: {[key: $Values<typeof TRANSFORM_TO_SHORT_KEY>]: TransformTypes} = { f: 'focus-subtree', ff: 'focus-function', mcn: 'merge-call-node', mf: 'merge-function', df: 'drop-function', cr: 'collapse-resource', rec: 'collapse-direct-recursion', };

Adding a new transform type wouldn't force you add it to these structures (I don't know how to do it, I think we'd need to check that $Keys<typeof TRANSFORM_TO_SHORT_KEY> === TransformTypes but I don't know if that's possible), but your current code doesn't either. But this keeps the 2 structures synced in a type-safe way, with a much simpler code.

Mainly I wanted it to throw an error when I add a new type, so that I know to go and hit that code and update it. It's not 100% foolproof. I tried yours and it's not giving me errors when I add a new transform type.

With the switch it handles the unexpected case:

I see, this is because you added the TransformType type to the argument (it wasn't there in the first commit), and Flow doesn't understand that not all types will be passed.
OK then.

julienw · 2018-01-22T15:37:45Z

src/profile-logic/transforms.js


 /**
 * This file contains the functions and logic for working with and applying transforms
 * to profile data.
 */

+// Create mappings from a transform name, to a url-friendly short name.
+export const TRANSFORM_TO_SHORT_KEY: { [TransformType]: string } = {};
+export const SHORT_KEY_TO_TRANSFORM: { [string]: TransformType } = {};


Why exporting them?

Not sure why, I removed export.

julienw · 2018-01-22T15:41:12Z

src/utils/flow.js

+    default: {
+      // The coerced type SHOULD be empty here. If in reality we get
+      // here, then it's not a valid transform type, so return null.
+      const unhandledCase: empty = coercedTabSlug; // eslint-disable-line no-unused-vars


nit: you can just do (coercedTabSlug: empty) and get rid of the unused var.
(see https://flow.org/en/docs/types/casting/#toc-type-assertions)

It's this eslint error or no-unused-expressions error

Adding it again so that it's embedded in a review:

Ah, I see. Then I'd suggest to go by the no-unused-expressions because it could be made to work with Flow assertions. I filed eslint/eslint#9877 for this, maybe you can also reference the issue here.

julienw · 2018-01-22T15:41:42Z

src/utils/flow.js

+    default: {
+      // The coerced type SHOULD be empty here. If in reality we get
+      // here, then it's not a valid transform type, so return null.
+      const unhandledCase: empty = coercedType; // eslint-disable-line no-unused-vars


as above, you can just write (coercedType: empty) and get rid of the unused var.

julienw · 2018-01-22T15:44:20Z

src/utils/flow.js

+ * This function takes a string and returns either a valid TabSlug or null, this doesn't
+ * throw an error so that any arbitrary string can be converted, e.g. from a URL.
+ */
+export function toValidTabSlug(tabSlug: any): TabSlug | null {


nit: please take a string as parameter and do just like in convertToTransformType :)

There's a toValidTabSlug in url-handling, do you think you can make it use this new function?

I'd prefer any, as then I can take null types and whatever I throw at it. I originally took a string to be more strict, but I believe I pass potentially null values in here, and it always does the right thing, not matter the type, so this solution is more general.

Thanks for catching the toValidTabSlug duplication.

gregtatum · 2018-01-22T20:53:48Z

Ok @julienw, review should be addressed. I rebased, and added on two new commits which I will squash when it's ready to merge.

julienw · 2018-01-23T13:17:49Z

src/components/calltree/ProfileCallTreeContextMenu.js

+          data={{ type: 'collapse-function-subtree' }}
+        >
+          <span className="profileCallTreeContextMenuIcon profileCallTreeContextMenuIconCollapse" />
+          {"Collapse function's subtree across the entire tree"}


I have no way to type the nice quote directly

Wikipedia tells me, on a Mac US Layout: Option + Shift + ]
Or just ' as a HTML entity.

and have never used it in my English writing

From Wikipedia: Both simplifications carried over to computer keyboards and the ASCII character set. However, although these are widely used due to their ubiquity and convenience, they are deprecated in contexts where proper typography is important.

not sure why we should make it more complicated than it needs to be.

complicated? Why complicated?
It's nicer, and maybe more important, it's proper typography. The ' is just a convenience from a time when we were using typewriters or using 7-bit ASCII.
And here I took the time to go to Wikipedia to be able to give you the character to use anyway (’), so you just can copy paste it :)

Embedding this in a review so that you don't miss it ^

julienw · 2018-01-23T13:26:53Z

src/profile-logic/transforms.js

-        // Do not throw an error, as we don't trust the data coming from a user.
-        console.error('Unrecognized transform was passed to the URL.', type);
-        break;
+        throw assertExhaustiveCheck(type);


I see; and after all this makes it quite clear when reading that we can throw here.

julienw · 2018-01-23T14:19:39Z

src/profile-logic/transforms.js

@@ -237,6 +237,11 @@ export function stringifyTransforms(transforms: TransformStack = []): string {
          'Expected to be able to convert a transform into its short key.'
        );
      }
+      // This switch breaks down each transform down into shared groups of what data


nit: "down" repeated, I believe the first should be removed ?

julienw · 2018-01-23T14:21:53Z

src/profile-logic/transforms.js

+      // they need as defined in src/types/transforms.js. For instance some transforms
+      // need only a funcIndex, while some care about the current implemention, or
+      // other pieces of data. The only reason why they are grouped together is to
+      // not repeat the same line of code.


Can you please also add a similar comment in parseTransforms ?
Also I believe the last sentence is not useful, but up to you.

julienw · 2018-01-23T14:26:26Z

src/url-handling.js

-      console.error(
-        'Unknown tab found, maybe a URL upgrader needs to be written.',
-        slug
-      );


I think we miss this error now. But I think the Flow check brings a better protection.

julienw

You'll need to rebase and get a green build before landing.
Thanks for this new transform !

Some nits that I'd like to see fixed:

the apostrophe
adding one more comment and fixing one
trading eslint error "no-unused-vars" for "no-unused-expressions"

julienw · 2018-01-23T14:31:19Z

src/utils/flow.js

@@ -36,14 +36,61 @@ export function immutableUpdate<T: Object>(object: T, ...rest: Object[]): T {
 * This function takes a string and returns either a valid TabSlug or null, this doesn't
 * throw an error so that any arbitrary string can be converted, e.g. from a URL.
 */
-export function toValidTabSlug(tabSlug: string): TabSlug | null {
-  switch (tabSlug) {
+export function toValidTabSlug(tabSlug: any): TabSlug | null {


With any Flow won't tell you if you try to pass, I don't know, an object, or a number, which could indicate a bug.
Currently it's string | void (not null).
I won't fight for this but I still think this would be better.

julienw · 2018-01-23T14:37:36Z

src/utils/flow.js

+    default: {
+      // The coerced type SHOULD be empty here. If in reality we get
+      // here, then it's not a valid transform type, so return null.
+      const unhandledCase: empty = coercedTabSlug; // eslint-disable-line no-unused-vars


Adding it again so that it's embedded in a review:

Ah, I see. Then I'd suggest to go by the no-unused-expressions because it could be made to work with Flow assertions. I filed eslint/eslint#9877 for this, maybe you can also reference the issue here.

julienw · 2018-01-23T14:38:08Z

src/components/calltree/ProfileCallTreeContextMenu.js

+          data={{ type: 'collapse-function-subtree' }}
+        >
+          <span className="profileCallTreeContextMenuIcon profileCallTreeContextMenuIconCollapse" />
+          {"Collapse function's subtree across the entire tree"}


Embedding this in a review so that you don't miss it ^

julienw · 2018-01-23T14:39:28Z

src/profile-logic/transforms.js

+    }
+  }
+  const newSamples = Object.assign({}, samples, {
+    stack: samples.stack.map(oldStack => {


Ah yes, thanks, got it

julienw · 2018-01-23T14:41:00Z

src/profile-logic/transforms.js

+          'Expected to be able to convert a transform into its short key.'
+        );
+      }
+      // This switch breaks down each transform down into shared groups of what data


(sorry if this is a dupe, but I don't see it so writing it again)
nit: down is repeated, I believe the first one should be removed.

gregtatum requested a review from mstange January 18, 2018 19:48

gregtatum changed the title ~~Collapse subtree2~~ Collapse functions subtree transform Jan 18, 2018

julienw requested changes Jan 22, 2018

View reviewed changes

gregtatum force-pushed the collapse-subtree2 branch from bce2123 to 858f77d Compare January 22, 2018 20:50

julienw reviewed Jan 23, 2018

View reviewed changes

julienw approved these changes Jan 23, 2018

View reviewed changes

gregtatum added 3 commits January 24, 2018 13:37

Make transforms exhaustively checked

416de7a

Add memoize-immutable type definition

6b5e72f

Collapse function subtree transform; resolves firefox-devtools#545

ec85fb7

gregtatum force-pushed the collapse-subtree2 branch from 858f77d to ec85fb7 Compare January 24, 2018 19:46

gregtatum merged commit baf1757 into firefox-devtools:master Jan 24, 2018

Collapse functions subtree transform #721

Collapse functions subtree transform #721

Conversation

gregtatum commented Jan 18, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatum Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatum commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 18, 2018 •

edited

Loading

gregtatum Jan 22, 2018 •

edited

Loading