Shopify / seafoam

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

Replace the various allocation nodes by a single node per new object #63

Closed eregon closed 1 year ago

eregon commented 1 year ago

Simple

class T
  attr_accessor :value, :v2
  def initialize(value, v2)
    @value = value
    @v2 = v2
  end
end

def foo(cond, n)
  T.new(cond, n)
end

loop do
  foo(rand < 0.5, rand < 0.5 ? 1 : 2)
end

Before: old

After: new


Multiple

class T
  attr_accessor :value, :v2
  def initialize(value, v2)
    @value = value
    @v2 = v2
  end
end

def foo(cond, n)
  [T.new(cond, n), T.new(cond, n)]
end

loop do
  foo(rand < 0.5, rand < 0.5 ? 1 : 2)
end

Before: old

After: new


Cyclic

class T
  attr_accessor :value, :v2
  def initialize(value, v2)
    @value = value
    @v2 = v2
  end
end

def foo(cond, n)
  a, b = T.new(cond, nil), T.new(cond, nil)
  a.v2 = b
  b.v2 = a
  a
end

loop do
  foo(rand < 0.5, rand < 0.5 ? 1 : 2)
end

Before: old

After: new

eregon commented 1 year ago

Looks like the tests are failing, I'll investigate.

I was thinking actually it'd be nice to just try to render every graph of every example+test BGV file (and check no error). It seems there is already a subset of that logic in specs.

eregon commented 1 year ago

Do you care about RuboCop? (https://github.com/Shopify/seafoam/runs/8097850440?check_suite_focus=true) IMHO it's way too picky in this configuration, and actually having some really bad suggestions in there (e.g., using next for no good reason, silly indentation, Do not use parallel assignment, Use the lambda method for multiline lambdas., ...).

eregon commented 1 year ago

Re RuboCop a good way to configure it might be to use https://github.com/Shopify/ruby-style-guide which seems to have much more sensible defaults.

chrisseaton commented 1 year ago

Do you care about RuboCop?

Lol Benoit you'd never let me get away with a PR to your repo that didn't meet your repo's existing coding standards. You'd tell me I should have enabled a pre-push check.

I don't care about RuboCop, beyond avoiding discussions about style, so it's already failed me here.

Can you fix this PR as per existing standards, then yeah I'll switch to Shopify's standard style in another PR.

eregon commented 1 year ago

Regarding RuboCop, I tried the Shopify config and some RuboCop defaults still get in the way needlessly. syntax_tree seems much better, https://github.com/eregon/seafoam/commit/94c17311932f8f4ca39f61cf424813214c9948c0.