Optimize combining light in ZoneView.getVisibleArea().
#3343
Conversation
- Remove the use of a granular lock when populating `allLightPathMap`. - Avoid converting `Path2D` back to `Area` until after all lights have been combined. - Create `allLightAreaMap` in the swing worker and return it from there. This avoids any future unintended concurrent interactions with `allLightAreaMap`, and makes it clear that it is only used in `ZoneView.getVisibleArea()`.
kwvanderlinde
force-pushed
the
visible-area-performance-improvements
branch
from
25d9900 to
c33deaa
Compare
|
On my system, this PR reduces contention and improves the multithreaded performance by about >6x in the case described in #3342 (> 40s to < 6.5s for map load). As far as that goes, it could be merged as-is. But the reason I made this a draft is that I've discovered the multithreading just doesn't scale well for lights. It makes sense when the number of lights is spread across many different lumens values, but in practice this won't be the case as the number of lights grows. That's because it's trivial to attach a light source to a token, but adding new lumens requires defining a new light type. So in cases that "matter", i.e., when there are many light sources, there will be relatively few lumens to split them up by, meaning there will be a lot of contention on I'll be toying with a different change that just removes multithreading in this case and makes it totally synchronous. By avoiding multithreading overhead and taking advantage of being able to mutate entries in |
|
I did some more testing. On my desktop with with a Intel Core i5-9400, this change gives the improvement mentioned in this PR. I've now also tried it on my laptop that has a AMD Ryzen 7 5700U. It does much better already with the existing implementation (~16 seconds) and only saves ~3 seconds. So the benefits may be pretty slim on some hardware, but it should still be a net win. But it's all seems pretty moot anyways. I've implemented the single-threaded alternative and I'm finding the same case only takes ~1 second on my desktop (~40x improvement) and just shy of 1.5 seconds on my laptop (~12x improvement). So I'm closing this PR in favour of the alternative which is faster, needs less code, and is better for maintenance we don't have to reason about concurrency. |
Identify the Bug or Feature request
Addresses #3342.
Description of the Change
Avoid as much contention as possible when combining light areas.
allLightPathMap.Path2Dback toAreauntil after all lights have been combined.allLightAreaMapin the swing worker and return it from there. This avoids any future unintended concurrentinteractions with
allLightAreaMap, and makes it clear that it is only used inZoneView.getVisibleArea().Possible Drawbacks
Assuming I've done the concurrency correctly, shouldn't be any drawbacks. If I messed up, all sorts of weird lighting glitches could result.
Documentation Notes
N/A
Release Notes
This change is