search

facebookincubator / cinder

Cinder is Meta's internal performance-oriented production version of CPython.
https://trycinder.com
Other
3.42k stars 122 forks source link

Make outputType recognize calls to built-in types #108

Closed tekknolagi closed 1 year ago

tekknolagi commented 1 year ago

Calling str(x) (where str is still the built-in type) should always return UnicodeExact. Use the built-in type map and TBuiltinExact to filter out subclasses, because subclasses can do whatever they want.

facebook-github-bot commented 1 year ago

@jbower-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 1 year ago

@carljm has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

carljm commented 1 year ago

Internally this is failing tests in test_cinderjit and test_perf_profiler when run with JIT enabled. Can you repro?

tekknolagi commented 1 year ago

Huh. I don't have a CI repro -- looks fine to me on my actions: https://github.com/tekknolagi/cinder/actions/runs/5115083456/jobs/9195993151

I can try running locally later.

Can you provide me with some more detail? What is the exact test invocation? Is it crashing? ...?

tekknolagi commented 1 year ago

It looks like test_unwatch_builtins is interfering with the LoadMethodModule tests for some reason. If that runs first, the latter fail (everything else was commented out). If test_unwatch_builtins is run in a subprocess the issue goes away. I haven't looked harder yet.

tekknolagi commented 1 year ago

What I don't understand is if I limit the JIT list to functions only present in the LoadModuleMethod tests and insert a hardware breakpoint inside my SSA change (which never fires), the tests still fail. So I'm not changing the codegen, the codegen is very limited in scope, and still, test failure.

tekknolagi commented 1 year ago

Oh actually if I comment out my changes entirely the tests still fail. So I think that test just needs to be run in a subprocess.

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

tekknolagi commented 1 year ago

Augh, hold on, sorry

facebook-github-bot commented 1 year ago

@tekknolagi has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 1 year ago

@carljm has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@carljm merged this pull request in facebookincubator/cinder@ae3f7c6dca5aaedc4b1ca6550948bb18141fea9c.