Remove INLINE pragma from generalCategory

[mpickering]

In module GHC.Unicode.Internal.Char.UnicodeData.GeneralCategory:

generalCategory contains a huge literal string but is marked INLINE, this will duplicate the string into any use site of generalCategory. In particular generalCategory is used in functions like isSpace and the literal gets inlined into this function which makes it massive.

My recommendation: Extract the literal into a top-level string (marked NOINLINE) and remove INLINE pragma from `generalCategory.

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9958/

[treeowl]

Marking the string NOINLINE seems like a no-brainier. Could you explain why you want to remove the INLINE from the function? Where might we see advantages/disadvantages?

[phadej]

The GHC.Unicode.Internal.Char.UnicodeData.GeneralCategory module doesn't exist in base-4.17. it's added in May 2022.

In base-4.17 generalCategory is implemented with FFI.

generalCategory :: Char -> GeneralCategory
generalCategory c = toEnum $ fromIntegral $ wgencat $ fromIntegral $ ord c

foreign import ccall unsafe "u_gencat"
  wgencat :: Int -> Int

(no INLINE pragma on that - left up to GHC to decide, I guess).

I'd say whether the new implementation needs tweaks to work similarly as prior is a bug fix.

I stand corrected, the new implementation is in proposal #59 (comment).

[treeowl]

I thought performance was generally out of scope for the CLC anyway, though I'd want their input on a change that sometimes changes order of growth for the worse.

[mpickering]

@treeowl The README states that "Changes which affect performance or laziness and similar are deemed visible. " and given the CLC approved this MR originally I think it's sensible to raise the discussion here.

[phadej]

@treeowl The README states that "Changes which affect performance or laziness and similar are deemed visible. " and given the CLC approved this MR originally I think it's sensible to raise the discussion here.

That is fair. I'd hope for some criteria that some bugfix-y changes could be discussed just on GitLab MRs (i.e. have lighter process), but maybe it's clearer if everything goes through "proposal process", even with the cost that completely unrelated people (like me, and @treeowl) comment on the issues.

[mpickering]

I suppose the issue with making it top-level is that currently the literal is an Addr#.

[phadej]

I suppose the issue with making it top-level is that currently the literal is an Addr#.

I was going to mention that. And if you wrap it into a Ptr, there will be indirection.

But AFAIK GHC can pull Addr# to the top-level (it does so for Typeable stuff e.g.

-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
Data.Bifunctor.Assoc.$tcAssoc3 :: ghc-prim:GHC.Prim.Addr#
[GblId,
 Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
         WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 30 0}]
Data.Bifunctor.Assoc.$tcAssoc3 = "Assoc"#

)
but if you add NOINLINE, then will it or not. IIRC the w/w will be done, and wrapper will be inlined, but worker (i.e. Addr#) will stay non-inlined?

So I'd say, make a change with {-# OPTIONS_GHC -ddump-simpl #-} and check what the Core looks like.

[mpickering]

Here is one implementation. If the CLC would prefer a different one then it would be good for someone else to prepare a patch. https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9958/

[Bodigrim]

Dear CLC members, let's vote on the proposal to prevent inlining of a huge binary literal bitmap# in Data.Char.generalCategory as per https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9958/. @phadej kindly checked that Core looks good now.

@angerman @chshersh @hasufell @mixphix @tomjaguarpaw @parsonsmatt

---

+1 from me.

[tomjaguarpaw]

+1

[hasufell]

+1

[angerman]

+1

[chshersh]

+1

---

W.r.t. to whether such changes should be under the CLC provision or not, I'm okay with keeping the status quo. Reviewing such proposals doesn't take much time (most heavy work is done by @mpickering and @phadej here), and it's still good to share knowledge about what's going on in base with more people so CLC members could make more informed decisions in the future as well 👍🏻