Shopify / seafoam

A tool for working with compiler graphs dumped by the GraalVM compiler
MIT License
126 stars 22 forks source link

Less aggresive color for loads and side effects #45

Closed eregon closed 2 years ago

eregon commented 2 years ago

I'm using:

def foo(s)
  /a/ =~ s
end

loop do
  foo("abc")
end

and

jt -u jvm-ce graph --method 'block in <main>' test.rb 

(needs https://github.com/oracle/truffleruby/compare/master...eregon:jt-graph-vm-args)

And only showing after the LoopBegin and what follows.

Seafoam: Screenshot from 2021-10-07 16-47-15

IGV: Screenshot from 2021-10-07 16-47-31

I find the bright red from Seafoam to be aggressive on the eyes and hurting readability of the produced graph. Red is usually used to bring attention but reading a field is nothing special and certainly not what's worth highlighting in this graph. The colors from IGV seem much better there, I find it so much easier to see the control flow in IGV, and part of it is using red only for "big control flow" like loops (LoopBegin/LoopExit/LoopEnd), If, Merge, Return, Invoke.

Seafoam uses bright red for load/store fields, deopts, etc but those are all pretty much normal in the graph and I think not worth highlighting specifically.

Could you use a less strong color for load/store fields, etc? I also thing Deopt TransferToInterpreter should be a different color, it's not really the same category as memory accesses. I think non-inlined calls should remain red as those are typically what's expensive.

eregon commented 2 years ago

@chrisseaton Any thought on this?

chrisseaton commented 2 years ago

Yes I'm open to it.

Do we want a library of colour schemes? Grey-scale for use in papers? High contrast? I've already done a colour-blindness consult on the current configuration to check it's suitable.

chrisseaton commented 2 years ago

Are you happy with this now?

eregon commented 2 years ago

Yes, now memory & control flow have different colors. I find the black for virtual objects a little bit too outstanding but colors are very subjective and it doesn't hurt readability. Probably we should group all allocation-related nodes in a single node for readability and that would likely solve it anyway.

Fixed by https://github.com/Shopify/seafoam/pull/59