Panicks when more than 16 lasers

[damienBloch]

I was trying to increase the number of cooling lasers to replicate sidebands.
Every time I add more than 16 lasers to the world, a thread panicks.

Any thoughts on what could be causing the problem ?

[ElliotB256]

Currently the laser samplers store per-laser quantities in a fixed-size array of length 16, see laser::COOLING_BEAM_LIMIT. We originally used a variable length (and heap allocated) Vec<> for laser samplers, but there was a performance increase for moving to fixed-size arrays. You can see more about the performance tweaks here.

Short fix, you could raise the cap (just modify the COOLING_BEAM_LIMIT). Long term, we can return to whether it's worth using Vec<> again (or some other faster container)

[ElliotB256]

Note that performance was faster with fixed arrays, but we could debate whether it's worth the flexibility. A few posts down, it says:

17.7s compared to 22.5s using vec!

[damienBloch]

Ok, thanks ! I'll have a look.

It just that the 16 limits seemed a bit low. A simple MOT already has 6 beams so it's fairly easy to go above 16.

[ElliotB256]

Yeah agreed, especially when you also have e.g. sidebands in some setups.

[ElliotB256]

Could you profile the benchmark simulation as a function of the array size?

[damienBloch]

I simply measured the duration time for the benchmark example on a i7-1165G7 @ 2.80GHz.
Not sure how to do a detailled profiling though.

I went a bit high for the upper limit because I want to have a MOT with many sidebands but I imagine it's an extreme case.

I had to remove #[derive(Serialize, Deserialize)] for ExpectedPhotonsScatteredVector and ActualPhotonsScatteredVector for this test, because Rust doesn't automatically implement these traits for arrays of size > 32.

[ElliotB256]

Thanks Damien, there's obviously an overhead with shuffling larger arrays around in memory (as expected). It looks like 32 would be a safe short term extension (which allows serialize,deserialize traits to be preserved), and we can return to whether a vec approach would be more suitable in other cases (I'd be interested to see how the overhead scales for vec versus larger cooling_beam_limit

[damienBloch]

Also maybe a more explicit error message could be helpful ?
Otherwise it's unclear why the number of entities with CoolingLight components is limited.

I also just noticed that the lasers are cached with a LASER_CACHE_SIZE in some files, but I didn't change this value in the previous runs.

[ElliotB256]

The LASER_CACHE_SIZE plays a different role. It defines a fixed size slice which we iterate over. It was more important in the original vec implementation, because it allowed us to process chunks of the vec in batches of a size known at compile time (and so enable SIMD floating point ops). Since moving to fixed size arrays, it's possible any performance gain is no longer worth the extra maintenance/complexity. However, let's bear it in mind when we return to profiling whether a fixed size/vec approach is worth it.

[minghuaw]

Have you considered using constant generics to pass the control over maximum number of lasers to the user while still having a fixed-length array?

[ElliotB256]

That's a great idea. There will be a rewrite at some point in the future to support other cooling transitions/models of scattering rates, and we should add the change then.

[zpbaldo]

Hi,
I can't seem to be able to add a generic number of beams if not by modifying also LASER_CACHE_SIZE to the desired beam number in every file it is contained, but I'm not sure it should be meant to be done this way.
What is the proper way to set a generic number of beams? Thanks!

[ElliotB256]

@zpbaldo I hadn't actually added constant generic support yet, although I do think minghuaw's suggestion is a good one. My intention has been to return to atomecs once bevy has stabilised a bit more (I was working on #3 after specs got deprecated; there's good benching data in there and to be fair it has now been a bit of time and bevy has matured, so I should return to this).

But this does also sound like a bug, as in #55 (comment) this should just refer to a batch size for looping through a more generalisable number of lasers. What specific setup are you encountering this on?

[zpbaldo]

@ElliotB256 Yes I read LASER_CACHE_SIZE is the batch size but I found that modifying it would allow me to add a generic number of beams in the simulation.
To reproduce that, one can run any example (1d mot for instance) and loop over a certain number of beams, say 128. It will panic and give something like this "thread '' panicked at 'range end index 32 out of range for slice of length 16'".
Changing the LASER_CACHE_SIZE in the files 'intensity.rs','doppler.rs', 'force.rs', 'sampler.rs' will allow to add beams until the set value.