JS: Refactor CFGExtractor.java

[erik-krogh]

I considered taking a look at #6362

But the code in CFGExtractor.java wasn't that readable.
So before I start fixing bugs, I went ahead and refactored CFGExtractor.java.

• There was a lot of Object types. I changed all of those to more descriptive types (mostly Collection<Node>).
  visitSequence still works with raw types. I don't think we would gain anything by refactoring that method any further.
• Lots of unclear names: hcaerof, V, seq, succ. I tried to use more descriptive names.

No behavior change.
An evaluation looks good (no increase in extraction time).

This is based on a refactor I tried to do last year but never finished.

[asgerf]

Yes!!! 💪 This is seriously excellent work! 👏

The one thing I'm not so sure about is rewriting some hot methods to have shorter but potentially slower implementations. End-to-end benchmarking is dominated by other factors like TRAP writing, so behind the noise we can't really see how the performance of the CFG extractor was affected. Would you mind benchmarking it more closely, or just reverting those particular changes?

[esbena]

dca can collect the data for us, but we need to make a custom report generator to display the diff.
If you generate an experiment with --uninterpreted-queries query-suites/javascript/uninterpreted-queries.qls, then the data for https://github.com/github/codeql/blob/1a078c38ad956091f87bf6cbffa15cedc3f380bd/javascript/ql/src/meta/extraction-metrics/PhaseTimings.ql will be available.

[erik-krogh]

dca can collect the data for us, but we need to make a custom report generator to display the diff.

I did that experiment on default.yml, and did a one-off report generator hack.
The CFG extraction got about 4.5% slower.
But the CFG extration was only about 4% of the total time spent (counting up the timings in PhaseTimings.ql).
So the expected total slowdown is 4% * 4.5% = 0.18%

I'll see if removing some streams and adding some more special cases helps.

[esbena]

This is O(n^2).

[erik-krogh]

I know, but n is always small.

I did an experiment trying to extract Microsoft/vscode, and n was at most 3.

My idea was to avoid allocating a HashSet.
Also, it's basically what the implementation did before my refactor.

[erik-krogh]

I did that experiment on default.yml, and did a one-off report generator hack.
The CFG extraction got about 4.5% slower.

The CFG extraction got slightly faster from the last few commits, but still around 3% slower than before my refactor.

[asgerf]

OK, good enough for me