compiler: Faster compilation time#2949
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2949 +/- ##
==========================================
- Coverage 83.47% 83.05% -0.43%
==========================================
Files 249 250 +1
Lines 52276 52949 +673
Branches 4503 4575 +72
==========================================
+ Hits 43638 43976 +338
- Misses 7880 8182 +302
- Partials 758 791 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mloubout
left a comment
mloubout
left a comment
There was a problem hiding this comment.
Not sure I like some of those intricate caching methods that could use better implementation rather than a new caching method every place it needs something a bit different.
|
|
||
|
|
||
| def ccode(expr, printer=None, **settings): | ||
| def get_printer(printer, dtype=None): |
There was a problem hiding this comment.
This seems to do a lot more than it needs including creating printers that will never be used. Why not just
@memoized_func
def get_printer(printer, dtype=None):
return printer(settings={'dtype': dtype})
| def __repr__(self): | ||
| return "Cluster([{}])".format(('\n' + ' '*9).join(f'{i}' for i in self.exprs)) | ||
|
|
||
| def __getattr__(self, name): |
There was a problem hiding this comment.
I'm not sure this is a cleaner way than either inheriting from EqBlock or not having EqBlock
There was a problem hiding this comment.
no, it must be a wrapper, it must not inherit. That's the whole point of unlocking caching
| """ | ||
|
|
||
| @classmethod | ||
| def _preprocess_args(cls, exprs, ispace=null_ispace, guards=None, |
|
|
||
| return Cluster(exprs, ispace, guards, properties, syncs, halo_scheme) | ||
|
|
||
| def rebuild(self, *args, **kwargs): |
There was a problem hiding this comment.
Seem to defeat the purpose not to rebuild just from _block.
| handle.update(updates) | ||
| return type(self)(**handle) | ||
|
|
||
| def _same_arg(self, key, value): |
There was a problem hiding this comment.
That looks like an odd bypass of memoized as well
| updates = OrderedDict([(k, v) for k, v in zip(argnames, args, strict=False)]) | ||
| updates.update(kwargs) | ||
|
|
||
| if updates and all(self._same_arg(k, v) for k, v in updates.items()): |
There was a problem hiding this comment.
Not sure how that same_arg is cheaper than just building the new object.
| if not same_children: | ||
| return o._rebuild(*children, **kwargs) | ||
|
|
||
| if kwargs and not all(o._same_arg(k, v) for k, v in kwargs.items()): |
There was a problem hiding this comment.
those _same_arg and same_as_before are still not super cheap i'm not sure why this would be better
|
|
||
| def pow_to_mul(expr): | ||
| if q_leaf(expr) or isinstance(expr, Basic): | ||
| if type(expr) in (tuple, list): |
There was a problem hiding this comment.
This should check if it's Iterable to handle generators and such
| if all(a is b for a, b in zip(expr.args, args, strict=False)): | ||
| args = tuple(args) | ||
|
|
||
| if type(expr) is tuple: |
There was a problem hiding this comment.
Again check Iterable and return type(expr)(args)
| body = iet.body._rebuild(body=iet.body.body + (DummyExpr(x, x),)) | ||
| return iet._rebuild(body=body), {} | ||
|
|
||
| monkeypatch.setattr(iet_engine, '_update_args', |
There was a problem hiding this comment.
Claude is driving me crazy with these monkeypatch 😅 it's always what it tries first for no reason. Like it had 80 of those or something in horizon
FabioLuporini
force-pushed
the
faster-python-rebased-final
branch
from
c38c64a to
21f8660
Compare
2 participants
the key ingredients are:
Bonus: a lot of, IMHO, good refactoring. And several new tests.
Compilation time improvements vary between 1.2x and 5x depending on the complexity of the Operator