Update L_segments to cache graphic parameters

[thomasp85]

This PR implements the same changes as done for points in #8

My own tests show that this change the performance from 6.75x to 3.14x relative to segments()

@pmur002 will you take a look at this

[pmur002]

Might be good to introduce a const for '15' in grid.h (since this will be appearing in a few more places as well).

[pmur002]

That looks like a fairly straightforward change! Did we get something right the first time around ?
If this checks out ok in my build/test, we could probably do several of these in one go as the next stage (?)

[pmur002]

Unfortunately, I am not seeing the same (relative) speed up ...

# R 3.4.1
  expression      min     mean   median     max `itr/sec` mem_alloc  n_gc n_itr
  <chr>      <bch:tm> <bch:tm> <bch:tm> <bch:t>     <dbl> <bch:byt> <dbl> <int>
1 base           17ms   21.1ms   18.6ms  39.8ms     47.4      7.7MB    40    24
2 grid          167ms  169.5ms  169.6ms 172.1ms      5.90    32.6MB    14     3

# R-devel (patched)
  expression      min     mean   median     max `itr/sec` mem_alloc  n_gc n_itr
  <chr>      <bch:tm> <bch:tm> <bch:tm> <bch:t>     <dbl> <bch:byt> <dbl> <int>
1 base         32.8ms   36.4ms   34.3ms  47.4ms     27.5         0B    2.    14
2 grid        258.7ms  263.3ms  263.3ms 267.9ms      3.80        0B    2.     2

My test code is ...

t <- seq(0, 360, length.out=1e5)
x <- .5 + .5*cos(t)
y <- .5 + .5*sin(t)
devoid::void_dev()
bench::mark(base =
                {
                    plot.new()
                    segments(x[-1e5], y[-1e5], x[-1], y[-1])
                },
            grid =
                {
                    grid.newpage()
                    grid.segments(x[-1e5], y[-1e5], x[-1], y[-1])
                },
            check = FALSE)

Am I doing something stupid ?

[pmur002]

On the other hand, WITHOUT the patch, R-devel is noticeably worse ...

  expression      min     mean   median     max `itr/sec` mem_alloc  n_gc n_itr
  <chr>      <bch:tm> <bch:tm> <bch:tm> <bch:t>     <dbl> <bch:byt> <dbl> <int>
1 base           33ms     34ms   33.9ms  35.8ms     29.4         0B    4.    11
2 grid          358ms    358ms  357.8ms 357.8ms      2.80        0B    1.     1

[thomasp85]

I can't really figure your setup out :-)... my way of testing is reinstalling grid in my standard installation, so that everything except grid is kept constant - this, of course, borkes my R installation somewhat but I can live with that for the moment. In that way I see a 2x speedup with the new segment implementation

To answer the first question. Yes, I believe we got it right with points :-) It's a general solution that should require a minimal set of changes to all primitives.

The reason we don't see the speed improvements that points gave is that point had additional changes to avoid converting the units for size more than once. Further, I suspect unit converting in general to be the main bottleneck right now and segments have twice the number of units than points so it might be masking some of the improvements...

[pmur002]

Yes, I'm beginning to think that my set up may not be ideal for benchmarking. This is such a small change (thanks to the previous patch) that I think it is reasonable to trust your timings (and even if they were wrong, we are not paying a big price in code complexity), so I have committed this patch now (r76245).

It may seem a bit hypocritical to talk about deadlines, but the R 3.6.0 release is probably going to be late April, which suggests a late March/early April feature/code freeze. For simplicity/coherency, so that all primitives move to this caching mechanism in the same R release, would it be possible to get a patch for all or most of the other primitives within about a week ? Only if they are as straightforward as this one and only if (heaven forbid) you don't have other things with higher priority :)

[thomasp85]

I'll begin to prepare a sweeping PR... I'm pretty sure that the only changes required are those done here, but will point out any additional if required.

How likely are you to take a look at #1 before the feature freeze? It would be nice to get such big changes into a minor release rather than a patch release, so they'd have to wait a year if we skip next release (I think)...

FWIW, I've made changes to minimise breaking changes due to implicit coercion of the unit vector (which incidentally have made it much faster to construct), so any breaking changes now should fully be due to package directly extracting attributes from the unit objects (which has never been safe anyway)

[pmur002]

Thanks for the cache primitives patch. I will try to start taking a proper look at #1 tomorrow. Yes, it would be great to get that into R 3.6.0 as well (sorry for not getting to it sooner!)