10 locale objects return references to internal structures not copies

[ccntrq]

This addresses #10

I figured I'll give this a try.

This should solve the described issue. I didn't find any other places where we leak internal structures to the outside without creating a copy.

In line 76: If I am not mistaken it would suffice to check if we have an ARRAY ref here and create a shallow clone of the array. I went for the dclone solution for increased safety/compatibility

[codecov]

Codecov Report

Merging #26 into master will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   79.42%   79.83%   +0.40%     
==========================================
  Files          16       17       +1     
  Lines         695      709      +14     
  Branches       77       77              
==========================================
+ Hits          552      566      +14     
  Misses        112      112              
  Partials       31       31              

Impacted Files	Coverage Δ
lib/DateTime/Locale/FromData.pm	92.13% <100.00%> (+0.08%)	⬆️
t/14locale-data-is-cloned.t	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcb1eb5...339884b. Read the comment docs.

[autarch]

Before I merge this I'd want to see a benchmark to ensure that this isn't a significant regression in speed for DateTime. While returning the internal refs is somewhat gross, in practice I don't think this is something that causes a lot of problems. I assume most people aren't really using the locale objects directly.

[ccntrq]

I setup a basic benchmark running day_name for some of the locales and pushed the benchmark here.

The results don't look very promising:

Running on master:

Benchmark: running locale_perf for at least 10 CPU seconds...
locale_perf: 11 wallclock secs (10.39 usr +  0.00 sys = 10.39 CPU) @ 499.71/s (n=5192)

Running on this branch:

Benchmark: running locale_perf for at least 10 CPU seconds...
locale_perf: 11 wallclock secs (10.51 usr +  0.00 sys = 10.51 CPU) @ 200.67/s (n=2109)

With a shallow clone of the array instead of using dclone:

Benchmark: running locale_perf for at least 10 CPU seconds...
locale_perf: 11 wallclock secs (10.81 usr +  0.00 sys = 10.81 CPU) @ 409.99/s (n=4432)

With a drop in performance of about 20% in the shallow clone case, I don't think this is worth pursuing.

We could a a comment to the affected code parts and document the behavior somewhere and then close this issue.

Whats your opinion on this?

[autarch]

It's a bit discouraging, but I'd suggest benchmarking DateTime instead. That's what people really care about performance-wise. I would expect the hit to be much smaller. It might even be so small that it's invisible.

It might also be worth investigating whether dclone is slower than doing it by hand with something like:

return ref($ref) eq 'ARRAY' ? [@{$ref}] : ref($ref) eq 'HASH' ? {%{$ref}} : die 'wtf is this ref?';

[autarch]

Actually, no need for runtime ref checks. Each of the methods can be told what the expected reference type is and could use a custom generated method. That could also speed things up.

[ccntrq]

Allright. I looked into this a little further.

I added seperate commits to talk about.

One, that adds a deep clone to the return of locale_data. I couldn't find a
use of that method in DateTime, so performance isn't critical here. It seems,
the main purpose of the method is to allow the user to quickly provide a
modified locale to register_from_data. For that reason I think protecting the
original locale from modification is more important.

Actually, no need for runtime ref checks. Each of the methods can be told what the expected reference type is and could use a custom generated method. That could also speed things up.

In the other commit the method generation in FromData is reworked according to
this suggestion. We only expect ARRAY refs or norefs. I added a method to
reftype map and a custom sub for the ARRAY methods. The method for the noref
case is the one from before.

The Benchmark now runs two methods from DateTime, one that internally uses a
method from FromData where we expect an ARRAY reftype and one where we
expect noref. In the noref case there is no notable performance penalty at all.
Allthough, in the ARRAY ref case we see a performance penalty of roughly 40-50%.

This not beeing a real world benchmark, how would you like to proceed on
this?

---

Benchmark Results:

This branch (abfabd47860398f9c987b379ffbe7bfb2545cb74):

Benchmark: running with_array_ref, without_ref for at least 10 CPU seconds...
with_array_ref: 10 wallclock secs (10.44 usr +  0.00 sys = 10.44 CPU) @ 1233658.24/s (n=12879392)
without_ref: 11 wallclock secs (10.48 usr +  0.00 sys = 10.48 CPU) @ 21194.56/s (n=222119)

On master

Benchmark: running with_array_ref, without_ref for at least 10 CPU seconds...
with_array_ref: 10 wallclock secs (10.60 usr +  0.00 sys = 10.60 CPU) @ 2205942.45/s (n=23382990)
without_ref: 10 wallclock secs (10.54 usr +  0.01 sys = 10.55 CPU) @ 21053.93/s (n=222119)

[autarch]

Yeah, a 50% speed hit is not really acceptable. That said, I agree that adding the dclone to locale_data is worth the hit.

[ccntrq]

I agree. Reworked the PR.

This now contains the changes to the locale_data method and a testcase to prove modifying that data doesn't affect the original locale. I also updated the pod accordingly.

Concerning #10 I drafted an addition to the pod to indicate that the returned array references have to be handled with care.

[ccntrq]

rebased to resolve conflict in lib/DateTime/Locale/FromData.pm

[autarch]

Merged from the CLI. Thanks!