Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more directives for core libraries #21

Closed
wants to merge 88 commits into from

Conversation

jam-awake
Copy link
Collaborator

Fixes #6. WIP.

I'm assuming that categorizing these directives by library will make it easier to determine whether we've fully audited that library.

@natefaubion
Copy link
Collaborator

Is there a justification for why you are adding the things you are adding? Some of them make sense to me, as they are combinators and it's helpful to inline to the more primitive call, but some of the dictionaries aren't clear to me. To be clear, I don't think it should be a goal to add an annotation for everything (this is unnecessary), but for things that are advantageous for optimizations.

Effect.Class.Console.clear arity=1

-- const
Data.Const.eqConst arity=1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you add all the const methods because they are essentially noops?

Data.Const.commutativeRingConst arity=1
Data.Const.heytingAlgebraConst arity=1
Data.Const.booleanAlgebraConst arity=1
Data.Const.applyConst arity=1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for things like this it might be better to use the new (..) syntax rather than inlining the dictionary directly.

Comment on lines 26 to 31
Data.CatList.foldMap arity=1
Data.CatQueue.fromFoldable arity=1
Data.CatQueue.cqEq arity=1
Data.CatQueue.cqCompare arity=1
Data.CatQueue.eqCatQueue arity=1
Data.CatQueue.ordCatQueue arity=1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there particular optimizations you are trying to trigger with these?

@jam-awake
Copy link
Collaborator Author

Is there a justification for why you are adding the things you are adding? Some of them make sense to me, as they are combinators and it's helpful to inline to the more primitive call, but some of the dictionaries aren't clear to me. To be clear, I don't think it should be a goal to add an annotation for everything (this is unnecessary), but for things that are advantageous for optimizations.

I figured I would at least get the PR started, even if it wasn't correct in various places. My methodology was just adding an inline directive for everything that had a type class constraint. I know that's not correct in various circumstances, but I don't have a clear understanding as to when it's desirable to add such a directive and when it's not.

This is my current understanding:

  • for functions derived from type class instances / combinators with type class constraints (e.g. voidLeft, lmap, etc.), it is always desirable to add an inline directive because you'll get a more specialized version of that function.
  • for primitive types with kind Type (e.g. String, Int), it's always desirable to add directives to their type class instances because those may trigger other optimizations
  • for primitive types with higher kinds like Type -> Type (e.g. Array, Record), it's sometimes desirable to add directives to their type class instances because those may trigger other optimizations. But because the resulting dictionaries such instances define take other dictionaries as arguments, it may lead to code bloat in some situations.
  • (based on your comments above about CatQueue). For data types that store other data types, it is generally undesirable to add directives to their type class instances as it may inline too much and bloat your code. Const is an exception to this because it ignores its arguments.

@natefaubion
Copy link
Collaborator

It sounds like you are wanting a consistent rule that you can apply to every definition based on it's type signature alone. Optimizations rely on the code that is generated (ie, the implementation). Sometimes things can have a simple signature, but inlining them doesn't result in anything interesting happening, and so you've just duplicated code to no effect. If you want to inline something, then yes, it's type signature can help you decide what directive to add, but the type signature alone does not tell you something is worth inlining. My recommendation is to not add a directive unless you have a specific desired result from that inlining. It is not interesting to inline something for the sake of inlining. Yes, this means that you can't just go through and add directives without observing the result. The original issues says:

We should assess if there are more things in core that would benefit users by being in the default set.

The question you have to answer is "is this beneficial?". If you aren't sure, then don't add it! This is why there are a bunch of "default rules" snapshots. They observe something practical happening from the default rules firing.

for functions derived from type class instances / combinators with type class constraints (e.g. voidLeft, lmap, etc.), it is always desirable to add an inline directive because you'll get a more specialized version of that function.

This is generally an alright rule, but no, there is no such thing as "always desirable" based on a type signature. These are generally good to inline because 1) they are often just an application of a more primitive form 2) they are usually small and don't contribute to code bloat.

for primitive types with kind Type (e.g. String, Int), it's always desirable to add directives to their type class instances because those may trigger other optimizations

No, this isn't really a rule I follow. What matters is if you can observe something beneficial happening by inlining them. It has nothing to do with the type.

for primitive types with higher kinds like Type -> Type (e.g. Array, Record), it's sometimes desirable to add directives to their type class instances because those may trigger other optimizations. But because the resulting dictionaries such instances define take other dictionaries as arguments, it may lead to code bloat in some situations.

The criteria for Record is that RowList metaprogramming is like a macro. it's pretty rare that RowList metaprogramming is not beneficial to inline because the structural induction gets flattened out to straightline code over record fields. But again, this has nothing to do with the type, and has to do with the code that gets generated. Regarding Array:

  • eqArray doesn't contribute to code bloat because it's a foreign implementation. There's no reason not to inline the direct foreign binding. But again, this is because of the implementation.
  • showArray is the same story. It's just a foreign implementation so no code bloat.

@@ -10,7 +8,7 @@ const test6 = -2147483648;
const test5 = 2147483647;
const test4 = /* #__PURE__ */ (() => Data$dOrd.ordString.compare("h")("i"))();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is another primitive operation that should be evaluated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #34 to track this.

@jam-awake
Copy link
Collaborator Author

This is ... tedious 😄

I still need snapshots for the following, but I'll get to that next week:

  • WriterT's MonadWriter usage
  • RWST
  • ListT
  • ContT (this will be fun...)
  • IdentityT

I'll likely skip the Comonad ones.

jam-awake added a commit that referenced this pull request Nov 1, 2022
Same as #21 but without the transformer code and the effect code
@jam-awake
Copy link
Collaborator Author

I've ported most of this code into #33. The current PR includes a lot of code for monad transformers, which revealed that more needs to be done to account for that (see above for comments). I've opened an issue to track the other issue I found for evaluating literal string comparisons.

Moreover, these might be annotations we shouldn't add to this project but leave it to end-users to do, especially with this project still being young.

@jam-awake jam-awake closed this Nov 1, 2022
jam-awake added a commit that referenced this pull request Nov 1, 2022
@jam-awake
Copy link
Collaborator Author

The monad transformers work was extracted and put into the jam/monad-transformers branch

@jam-awake jam-awake deleted the jam/add-more-core-directives branch November 1, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more inlining directives from across core.
2 participants