Skip to content

Conversation

bpmael
Copy link

@bpmael bpmael commented Sep 18, 2025

problem
extending Base.printstyled for AnnotatedIOBuffer in StyledStrings.Legacy causes many method invalidations due to specialization on msg....

root cause
the variadic msg... argument leads to excessive method instances, which get invalidated when a new method is added.

solution
add @nospecialize(msg...) to avoid unnecessary specialization while keeping behavior unchanged.

impact

  • fewer invalidations
  • keeps compatibility
  • minimal change (just one annotation)

testing

  • existing tests pass
  • printstyled works with AnnotatedIOBuffer
  • no slowdown in typical usage

@bpmael
Copy link
Author

bpmael commented Sep 18, 2025

sorry this fix for JuliaLang/julia#59592

@timholy
Copy link
Member

timholy commented Sep 18, 2025

FYI the invalidations in JuliaLang/julia#59592 are cause by the insertion of this method, so I don't think this fixes that issue.

However, this change should, I think, make it less likely that things using this method will themselves be invalidated by loading other packages. So it's not necessarily a bad change on its own, if there really isn't any measurable performance hit.

@bpmael
Copy link
Author

bpmael commented Sep 18, 2025

FYI the invalidations in JuliaLang/julia#59592 are cause by the insertion of this method, so I don't think this fixes that issue.

However, this change should, I think, make it less likely that things using this method will themselves be invalidated by loading other packages. So it's not necessarily a bad change on its own, if there really isn't any measurable performance hit.

thanks for the clarification, you're absolutely right, i was targeting the #59592 invalidations but seems like i ended up fixing a different (though related) issue instead

since this change does help with reducing future invalidation risks from other packages, should i

  • update the pr description to reflect the actual benefit rather than claiming it fixes #59592?
  • run some benchmarks to ensure no performance regression?
  • or would you prefer i investigate the root cause of #59592 invalidations more directly first?

happy to pivot the approach based on what would be most valuable for the codebase

@tecosaur
Copy link
Member

Thanks for this @bpmael! Invalidations here has been a (if not the) major headache, and so help improving the situation is much appreciated 🙂

I do think an inference barrier is needed here to properly sort out the issue, and so I've implemented that together with the @nospecialize addition you raise here in #124.

By the way, just from a read of the PR description, I'm inclined to guess that it was written by an LLM. Is that the case (not a problem) or am I imagining it?

@bpmael
Copy link
Author

bpmael commented Sep 18, 2025

Thanks for this @bpmael! Invalidations here has been a (if not the) major headache, and so help improving the situation is much appreciated 🙂

I do think an inference barrier is needed here to properly sort out the issue, and so I've implemented that together with the @nospecialize addition you raise here in #124.

By the way, just from a read of the PR description, I'm inclined to guess that it was written by an LLM. Is that the case (not a problem) or am I imagining it?

sorry in advance, the description was made by llm, because im not very good at putting words together 😅, thank you for accepting the patch, im glad i could help

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.

3 participants