chart-diagrams: Port to diagrams-1.3 #83

bgamari · 2015-05-06T19:22:48Z

This ports the diagrams backend to diagrams-1.3 (as well as cleans up some style issues). I have yet to test with GHC 7.10 due to GHC bug #10288.

bgamari · 2015-05-06T21:07:51Z

This appears to work fine with the current state of the ghc-7.10 branch and diagrams/diagrams-svg#80 (which fixes a bug in diagrams-svg).

bgamari · 2015-05-06T21:10:44Z

Actually, it seems like the colors are off in some of the EPS output (particularly tests 8, 9, 9c, 9l, and 9r). Is this a known issue with previous releases?

byorgey · 2015-05-07T01:23:15Z

Released the bug fix for diagrams-svg to Hackage.

timbod7 · 2015-05-25T12:21:06Z

See question on the related pull requests: #82

idontgetoutmuch · 2015-05-31T06:54:13Z

What's the news on this and on #82?

timbod7 · 2015-05-31T10:55:38Z

I've merged this with master, in an attempt to use the newly unified test harness with it. The results are in

https://github.com/timbod7/haskell-chart/tree/diagrams-1.3

unfortunately, the diagrams related modes of the harness all freeze, eg:

harness drawing-diagrams

Have I done something silly?

byorgey · 2015-05-31T19:09:02Z

I could try to look into this. Can you give me instructions on how to build & run the test harness?

timbod7 · 2015-05-31T22:13:57Z

The test harness can be built with cabal build in the chart-tests directory.

The make tests target in the top level makefile captures the steps required to build the harness and generate all test output.

To see the problem you'll need to be on the diagrams-1.3 branch.

byorgey · 2015-06-01T00:50:19Z

chart-diagrams/Graphics/Rendering/Chart/Backend/Diagrams.hs

                                          }
        makeSvgFont font usedGs
        -- M.Map (String, FontSlant, FontWeight) (S.Set String)
        -- makeSvgFont :: (FontData, OutlineMap) -> Set.Set String -> S.Svg
-    svg = D.renderDia DSVG.SVG (DSVG.SVGOptions (D2.Dims w h) fontDefs) d
+    svg = D.renderDia DSVG.SVG (DSVG.SVGOptions (D2.dims2D w h) Nothing T.empty) d


What happened to the fontDefs?

Ahem, right, I needlessly dropped it. Good catch.

byorgey · 2015-06-01T01:00:33Z

OK, thanks. Building now. I also took a careful look through all the diffs in this pull request and everything looks fairly straightforward.

byorgey · 2015-06-01T01:18:42Z

For me it does not hang, though it is rather slow (took several minutes to finish running harness drawing-diagrams).

byorgey · 2015-06-01T01:20:13Z

If that is much slower than it used to be then perhaps we should look into why it is so slow.

timbod7 · 2015-06-01T13:04:33Z

It does indeed seem to be much slower - i just assumed it had frozen!

master:

22:58:39 {master} ~/repos/chart$ cabal sandbox hc-pkg list | grep diagrams
    Chart-diagrams-1.4
    diagrams-cairo-1.2.0.7
    diagrams-core-1.2.0.6
    diagrams-lib-1.2.0.9
    diagrams-postscript-1.1.0.5
    diagrams-svg-1.1.0.5
22:59:04 {master} ~/repos/chart$ time ./.cabal-sandbox/bin/harness drawing-diagrams

real    0m5.970s
user    0m5.514s
sys 0m0.399s
22:59:33 {master} ~/repos/chart$

diagrams-1.3:

22:58:42 {diagrams-1.3} ~/repos/chart-bgamari$ cabal sandbox hc-pkg list | grep diagrams
    Chart-diagrams-1.4.1
    diagrams-cairo-1.3.0.2
    diagrams-core-1.3.0.1
    diagrams-lib-1.3.0.1
    diagrams-postscript-1.3.0.1
    diagrams-solve-0.1
    diagrams-svg-1.3.1.2
22:59:39 {diagrams-1.3} ~/repos/chart-bgamari$ time ./.cabal-sandbox/bin/harness drawing-diagrams

real    3m16.784s
user    3m4.017s
sys 0m8.995s
23:03:07 {diagrams-1.3} ~/repos/chart-bgamari$

byorgey · 2015-06-01T13:37:57Z

Ouchies! That is comically bad. Let's see if we can figure out what is going on. Later today I will see if I can do some profiling.

bgamari · 2015-06-01T13:48:20Z

Sorry for the latency! My laptop has had an unfortunate incident involving a glass of tea and consequently internet access will be a bit spotty for a few days. As I recall things were indeed quite slow.

bergey · 2015-06-01T16:39:08Z

It was easy to narrow the problem down to SVGFonts. defaultFonts in chart-diagrams calls loadFont in SVGFonts, which accounts for an impressive 99.9% of the runtime (and allocations) for harness drawing-diagrams. I commented out most of defaultFonts to only load one font - this brings the runtime down by a factor of 20 (10m39s to 36s), but 98% of the time is still in loadFont. I was hoping this change would give enough resolution to notice any other slow sections, but maybe it's just the one.

Call tree for the unmodified test suite (search for 99.9)
Call tree with only one font loaded (search for 97.8)
(4 MB each)

Beyond that, I'm not sure what's changed. I don't see any changes to font handling in chart-diagrams recently. I've never really looked at the code for SVGFonts, so I hope @tkvogt, @jeffreyrosenbluth or someone else can look more closely.

tkvogt · 2015-06-01T17:19:30Z

I can maybe fix this. My plan was to make SVGFonts depend on diagrams-input, because the svg parser in diagrams-input has a more correct path handling (d="M..") and is faster because of attoparsec and xml-conduit. Then half of the code in SVGFonts will vanish and maybe this bug will also go away.

bgamari · 2015-06-01T17:57:44Z

@tkvogt @bergey right, now this is coming back to me. I do remember tracing this back to SVGFonts. I have a branch moving the parsing over to attoparsec and adding some strictness although don't recall it making a large difference. Unfortunately the branch is on the m2 drive of my ailing laptop at the moment so it's not terribly accessible.

I think SVGFonts could be made faster although I also think that Chart-diagrams could be a bit more conservative about how many fonts it asks for. As it stands defaultFonts unconditionally loads ten fonts regardless of whether the diagram even has any text.

byorgey · 2015-06-01T18:55:16Z

Is it possible that the call to loadFont is getting inlined somehow, so that the actual work of loading the font is happening many times?

tkvogt · 2015-06-01T19:48:56Z

@bgamari Did you change xml-parsing or path parsing, or both in your branch?
The path parsing has to be replaced, because there is an open bug: diagrams/SVGFonts#15
It's quite likely that the path parsing in diagrams-input is now fully correct.
I also think that the xml library of Galois is slower than xml-conduit because it goes trough the file (sub-) streams repeatedly while xml-conduit uses just one pass.

bgamari · 2015-06-01T19:54:11Z

@tkvogt only path parsing as far as I can recall.

bgamari · 2015-06-01T20:03:55Z

For the record, I've rebased against master and squashed some of the commits together.

byorgey · 2015-06-03T15:20:37Z

As a data point here, I instrumented loadDataFont in Graphics.SVGFonts.Fonts by changing the definition to

loadDataFont = trace "loadDataFont" (unsafePerformIO . loadFont . dataFile)

Now I can see that loadDataFont is being called repeatedly, in fact, once per call to textSVG! I am not sure of the proper way to make sure that it only gets called once and the result cached. I suspect that the type class polymorphism is not helping anything. The unsafePerformIO is not helping anything either. It is a perfectly valid use of unsafePerformIO from a semantic point of view, but from an operational point of view it is problematic.

byorgey · 2015-06-03T15:29:40Z

Correction: loadDataFont is called repeatedly when compiling the final executable with no optimization turned on. With -O2, loadDataFont only gets called once.

byorgey · 2015-06-03T15:31:51Z

I'm going to open an issue on the SVGFonts repo, we can keep discussing it there.

tkvogt · 2015-06-03T15:43:10Z

Could it be that simple:
The compiler can't recognize if a function is called again and again with the same argument.
One way to cache a result is to put it into a function without a parameter:
lin = loadDataFont "fonts/LinLibertine.svg"
We maybe have to use memoization.
Just my guess.

byorgey · 2015-06-03T16:26:03Z

Yes, but lin = loadDataFont "fonts/LinLibertine.svg" is not enough, because (1) it still has a typeclass polymorphic type, which means it may get re-run every time it is instantiated with a dictionary, and (2) it may get inlined. (Incidentally, I did try putting a NOINLINE pragma on lin but it didn't seem to make any difference.)

bgamari · 2015-06-09T12:19:00Z

Any outcome here?

@tkvogt you'll find a pull request for the attoparsec rework above.

tkvogt · 2015-06-09T13:18:12Z

I am in the middle of merging SVGFonts into the svg parser of diagrams-input, beacuse they logically belong together. But I don't want to let you wait for this. As mentioned here I will try a global lookup, test it and then upload SVGFonts to hackage.

bgamari · 2015-06-11T18:01:55Z

@timbod7 what do you think about just merging this? As far as I know the only issues are performance problems that can be dealt with later here or in SVGFonts.

timbod7 · 2015-06-11T21:58:41Z

I want to see this in as much as anyone...

However, It's 2 orders of magnitude slower than the existing diagrams backend, which is already significantly slower than the cairo backend. Even the tests don't finish in a reasonable amount of time.

bgamari · 2015-06-12T15:51:30Z

Fair enough.

timbod7 · 2015-06-14T13:48:38Z

Not wanting to completely sideline the good work done on this so far, I spend a couple of hours tinkering this evening. There definitely seems to have been some performance regression in SVGFonts. This is exacerbated by the fact that one test harness inadvertently loads the fonts multiple times. I fixed that harness, and I've made a couple of API breaking changes to further reduce the impact of font loading:

By default now only sans-serif fonts are loaded (ie 6 faces are loaded instead of 14).
There is now an API to let the caller preload all fonts, giving explicit control.

These go some way to addressing the immediate problem, but I'm still unsure about what a pure API for font selection and text rendering should look like.

Given the API change, the version will need to bump to 1.5 before a release.

timbod7 mentioned this pull request May 25, 2015

Support diagrams-1.3 #82

Closed

byorgey reviewed Jun 1, 2015
View reviewed changes

bgamari force-pushed the diagrams-1.3 branch from ec0388c to 88ca5e7 Compare June 1, 2015 19:47

bgamari added 3 commits June 1, 2015 15:52

diagrams: Whitespace cleanup

2f67815

Geometry: Fix haddock spelling

6b699c6

Geometry: Whitespace cleanup

1a236e1

bgamari force-pushed the diagrams-1.3 branch from 88ca5e7 to be254b6 Compare June 1, 2015 20:03

bgamari added 2 commits June 1, 2015 16:25

Bump lens upper bound

aaf4db0

Fix tests/drawing directory case

54432cb

diagrams: Port to diagrams-1.3

6ba7cd4

bgamari force-pushed the diagrams-1.3 branch from be254b6 to 6ba7cd4 Compare June 1, 2015 20:43

bgamari mentioned this pull request Jun 1, 2015

Don't ignore fontDefs #85

Closed

byorgey mentioned this pull request Jun 3, 2015

Fonts are loaded repeatedly diagrams/SVGFonts#16

Closed

bgamari mentioned this pull request Jun 9, 2015

Port path parser to attoparsec diagrams/SVGFonts#17

Merged

timbod7 merged commit 6ba7cd4 into timbod7:master Jun 14, 2015

timbod7 mentioned this pull request Jun 16, 2015

diagrams usage needs to be migrated to use the new 1.3 API #80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chart-diagrams: Port to diagrams-1.3 #83

chart-diagrams: Port to diagrams-1.3 #83

bgamari commented May 6, 2015

bgamari commented May 6, 2015

bgamari commented May 6, 2015

byorgey commented May 7, 2015

timbod7 commented May 25, 2015

idontgetoutmuch commented May 31, 2015

timbod7 commented May 31, 2015

byorgey commented May 31, 2015

timbod7 commented May 31, 2015

byorgey Jun 1, 2015

bgamari Jun 1, 2015

byorgey commented Jun 1, 2015

byorgey commented Jun 1, 2015

byorgey commented Jun 1, 2015

timbod7 commented Jun 1, 2015

byorgey commented Jun 1, 2015

bgamari commented Jun 1, 2015

bergey commented Jun 1, 2015

tkvogt commented Jun 1, 2015

bgamari commented Jun 1, 2015

byorgey commented Jun 1, 2015

tkvogt commented Jun 1, 2015

bgamari commented Jun 1, 2015

bgamari commented Jun 1, 2015

byorgey commented Jun 3, 2015

byorgey commented Jun 3, 2015

byorgey commented Jun 3, 2015

tkvogt commented Jun 3, 2015

byorgey commented Jun 3, 2015

bgamari commented Jun 9, 2015

tkvogt commented Jun 9, 2015

bgamari commented Jun 11, 2015

timbod7 commented Jun 11, 2015

bgamari commented Jun 12, 2015

timbod7 commented Jun 14, 2015

chart-diagrams: Port to diagrams-1.3 #83

chart-diagrams: Port to diagrams-1.3 #83

Conversation

bgamari commented May 6, 2015

bgamari commented May 6, 2015

bgamari commented May 6, 2015

byorgey commented May 7, 2015

timbod7 commented May 25, 2015

idontgetoutmuch commented May 31, 2015

timbod7 commented May 31, 2015

byorgey commented May 31, 2015

timbod7 commented May 31, 2015

byorgey Jun 1, 2015

Choose a reason for hiding this comment

bgamari Jun 1, 2015

Choose a reason for hiding this comment

byorgey commented Jun 1, 2015

byorgey commented Jun 1, 2015

byorgey commented Jun 1, 2015

timbod7 commented Jun 1, 2015

byorgey commented Jun 1, 2015

bgamari commented Jun 1, 2015

bergey commented Jun 1, 2015

tkvogt commented Jun 1, 2015

bgamari commented Jun 1, 2015

byorgey commented Jun 1, 2015

tkvogt commented Jun 1, 2015

bgamari commented Jun 1, 2015

bgamari commented Jun 1, 2015

byorgey commented Jun 3, 2015

byorgey commented Jun 3, 2015

byorgey commented Jun 3, 2015

tkvogt commented Jun 3, 2015

byorgey commented Jun 3, 2015

bgamari commented Jun 9, 2015

tkvogt commented Jun 9, 2015

bgamari commented Jun 11, 2015

timbod7 commented Jun 11, 2015

bgamari commented Jun 12, 2015

timbod7 commented Jun 14, 2015