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

SVG backend has broken text fusion #111

Open
isovector opened this issue Jul 9, 2019 · 5 comments
Open

SVG backend has broken text fusion #111

isovector opened this issue Jul 9, 2019 · 5 comments

Comments

@isovector
Copy link

isovector commented Jul 9, 2019

I just started playing with diagrams again, and it's pretty great!

However, I'm running into speed issues. I have an example project here. This thing builds up maybe 900 circles, and does some transformations on them.

It takes about 9 seconds to run on my machine, even with -O2. This is too slow for any kind of live coding experience, so I decided to do some profiling. The results are here:

COST CENTRE            MODULE                             SRC                                             %time %alloc  ticks     bytes

toText                 Graphics.Svg.Path                  src/Graphics/Svg/Path.hs:26:1-63                 19.3   24.7   2600 1931550016
fmap                   Linear.V2                          src/Linear/V2.hs:131:3-34                        12.8   12.1   1719 946285224
atParam                Diagrams.Segment                   src/Diagrams/Segment.hs:(232,3)-(236,18)         10.5   10.4   1411 813761968
getEnvelope.\          Diagrams.Segment                   src/Diagrams/Segment.hs:(295,5)-(301,36)         10.2    7.7   1376 600342736

and the cost centers:

                                                                                                                                                                                        individual      inherited
COST CENTRE                                                          MODULE                               SRC                                                      no.       entries  %time %alloc   %time %alloc  ticks     bytes

MAIN                                                                 MAIN                                 <built-in>                                               13590          0    0.0    0.0   100.0  100.0      2     67600
                   withTrail                                         Diagrams.Trail                       src/Diagrams/Trail.hs:811:1-54                           44663        924    0.0    0.0    21.6   30.1      0         0
                    withTrail'                                       Diagrams.Trail                       src/Diagrams/Trail.hs:(417,1)-(418,40)                   44664        924    0.0    0.0    21.6   30.1      1         0
                     renderTrail.renderLoop                          Graphics.Rendering.SVG               src/Graphics/Rendering/SVG.hs:(113,5)-(120,10)           44665        924    0.9    3.6    21.6   30.1    119 282848416
                      renderSeg                                      Graphics.Rendering.SVG               src/Graphics/Rendering/SVG.hs:(123,1)-(128,67)           44737      52234    0.0    0.0    20.4   26.3      5       240
                       cR                                            Graphics.Svg.Path                    src/Graphics/Svg/Path.hs:(68,1)-(70,53)                  44856      52230    1.2    1.7    20.3   26.3    160 133733280
                        toText                                       Graphics.Svg.Path                    src/Graphics/Svg/Path.hs:26:1-63                         44858     313380   19.1   24.6    19.1   24.6   2578 1918235944

Dang! Looks like text fusion is broken. The implementation of toText suggests why:

toText :: RealFloat a => a -> Text
toText = toStrict . toLazyText . formatRealFloat Fixed (Just 4)

formatRealFloat returns a Data.Text.Lazy.Builder, but then the builder is immediately forced. Later, every other function in this module just Text.concats them together! But all of these are strict texts! No wonder we're missing out on fusion and everything is allocating super hard!

The fix as best I can tell is to make toText return a Builder, and then replace each instance of T.concat in Graphics.SVG.Path with toStrict . toLazy . mconcat. Or better yet, to use this same builder all the way through the SVG generation.

@isovector
Copy link
Author

I'd be happy to send a partial fix, but it appears https://github.com/diagrams/svg-builder is unmaintained, and I'd hate to go through the effort just for a patch to be ignored.

@isovector
Copy link
Author

Something really odd is going on here.

toText returns a strict Text. But the entire diagrams-svg pipeline is in terms of a lazy bytestring builder! The only reason I can tell for the strict text is that it's used by Element, which is a HashMap (Text, Text) -> LBS.Builder. But the implementation just Blaze.ByteString.Builder.fromText. So why is there any Text in here at all??

@byorgey
Copy link
Member

byorgey commented Jul 10, 2019

These days I can't give the time or attention to diagrams I would like, but it's definitely not unmaintained. A patch would be welcome and not ignored.

I confess I don't know much about svg-builder or the SVG backend. They were originally written by @jeffreyrosenbluth but he probably doesn't remember either. In any case there's probably no hidden reason for the existence of Text --- probably just an oversight.

@isovector
Copy link
Author

Makes sense, thanks for the info. I'll try to put some more effort into this tomorrow.

@jeffreyrosenbluth
Copy link
Member

Indeed I don't remember the reason Text is involved. Perhaps it had to do with using formatRealFloat from the Text package - if i recall only returns lazy text. It might not be a bad idea to roll your own version of formatRealFloat for our limited use case of converting a floating point number to a string/builder with 4 decimal points of precision. Maybe even something as simple as this would be an improvement:

show4 :: RealFloat a => a -> String
show4 a = show n ++ ('.' : show f')
  where
    (n, f) = properFraction a
    f' = abs . round $ 10000 * f

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

No branches or pull requests

3 participants