Implement GetStripedOffsets

[mstoykov]

The idea of this PR is to

1. Basic review of the algorithm - it is fairly short so it shouldn't take long and the basics of the implementations are hardly going to change.
2. Discuss whether it should be implemented as it is or should we basically calculate it for every segment so that it can be used in a future distributed mode? It really won't be much more code and will add some memory overhead but nothing big really.
3. I think the returned values should be a list offsets(where the first one is the start) and the second the LCD, as you would otherwise need to calculate it again ... and there is really no point in having start and offsets as separate values.

[codecov-io]

Codecov Report

Merging #1323 into new-schedulers will increase coverage by 0.12%.
The diff coverage is 86.59%.

@@                Coverage Diff                 @@
##           new-schedulers    #1323      +/-   ##
==================================================
+ Coverage           76.30%   76.42%   +0.12%     
==================================================
  Files                 161      161              
  Lines               12625    12799     +174     
==================================================
+ Hits                 9633     9782     +149     
- Misses               2482     2500      +18     
- Partials              510      517       +7     

Impacted Files	Coverage Δ
cmd/run.go	8.73% <0.00%> (ø)
cmd/options.go	63.33% <10.00%> (-3.81%)	⬇️
lib/options.go	87.69% <14.28%> (-5.72%)	⬇️
lib/executor/externally_controlled.go	58.82% <60.00%> (ø)
core/local/local.go	70.17% <66.66%> (-0.50%)	⬇️
lib/executor/variable_arrival_rate.go	96.20% <89.47%> (ø)
lib/execution_segment.go	91.05% <93.52%> (+4.15%)	⬆️
lib/execution.go	91.79% <100.00%> (+0.06%)	⬆️
lib/executor/constant_arrival_rate.go	97.35% <100.00%> (+0.31%)	⬆️
lib/executor/constant_looping_vus.go	96.42% <100.00%> (-0.05%)	⬇️
... and 8 more

---

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 b14db8e...c2f3163. Read the comment docs.

[na--]

I think you're calculating the offsets in a way that's not very useful. My original idea with the algorithm was to have a start and a slice of increments/offsets, but instead of offsets being the specific indexes i, for them to be numbers that you could easily use in a for loop.

See how I calculated this here: https://play.golang.org/p/NFTzwlHTXFw

[na--]

This would have to be validated somewhere higher up as well, after the options consolidation. This check would just be insurance, I guess, so I think there's no need

[na--]

Basically, my increments from the above example has length of len(offsets)+1, and :

• increments[0] = offsets[0] - start
• increments[1] = offsets[1] - offsets[0]
• ...
• increments[n+1] = denominator + start - offsets[n]` (I think? basically, how long you have to wrap around from the last offset back to the start)

[mstoykov]

There are benchstat outputs in each commit after the benchmarks were added but from the first to the current one the results are :

name                                   old time/op    new time/op    delta
GetStripedOffsets/length10,seed777-4     58.9µs ±41%    28.8µs ±31%  -51.20%  (p=0.000 n=50+50)
GetStripedOffsets/length100,seed777-4    19.3ms ±19%     4.8ms ±14%  -75.25%  (p=0.000 n=49+50)
GetStripedOffsetsEven/length10-4         10.4µs ± 2%     4.9µs ± 3%  -52.23%  (p=0.000 n=43+44)
GetStripedOffsetsEven/length100-4         395µs ± 2%      58µs ± 4%  -85.38%  (p=0.000 n=43+43)
GetStripedOffsetsEven/length1000-4       32.5ms ± 2%     0.9ms ± 6%  -97.19%  (p=0.000 n=43+43)

name                                   old alloc/op   new alloc/op   delta
GetStripedOffsets/length10,seed777-4     21.7kB ±54%    10.8kB ±22%  -50.25%  (p=0.000 n=50+49)
GetStripedOffsets/length100,seed777-4    9.82MB ±21%    0.66MB ± 9%  -93.28%  (p=0.000 n=48+50)
GetStripedOffsetsEven/length10-4         3.93kB ± 0%    3.19kB ± 0%  -18.74%  (p=0.000 n=50+50)
GetStripedOffsetsEven/length100-4         116kB ± 0%      36kB ± 0%  -69.32%  (p=0.000 n=50+50)
GetStripedOffsetsEven/length1000-4       8.35MB ± 0%    0.34MB ± 0%  -95.95%  (p=0.000 n=46+45)

name                                   old allocs/op  new allocs/op  delta
GetStripedOffsets/length10,seed777-4      1.07k ±42%     0.34k ±25%  -68.09%  (p=0.000 n=50+50)
GetStripedOffsets/length100,seed777-4      389k ±17%       22k ±10%  -94.27%  (p=0.000 n=48+50)
GetStripedOffsetsEven/length10-4            233 ± 0%        87 ± 0%  -62.66%  (p=0.000 n=50+50)
GetStripedOffsetsEven/length100-4         11.4k ± 0%      0.9k ± 0%  -92.26%  (p=0.000 n=50+50)
GetStripedOffsetsEven/length1000-4        1.01M ± 0%     0.01M ± 0%  -99.17%  (p=0.000 n=50+50)

[imiric]

A few minor things and nitpicks. Evaluating the correctness of the algorithm is a bit difficult for me right now as this functionality is not used yet, but I'll compare it to the spreadsheet and play around with it some more.

[imiric]

Typos?

[na--]

Suggested change

	// here the we need the not scaled one
	// here the we need the not-scaled one

or

Suggested change

	// here the we need the not scaled one
	// here the we need the unscaled one

[na--]

I'm wondering if we should leave that one undocumented for now, considering that it might confuse people a lot? There's a Hidden field in pflag.Flag, though we should probably just write a proper long tutorial for how execution segments work after we release them...

[na--]

A little bit bikesheddy, but why not just use ExecutionSegmentSequence as the variable name as well? 😅

[mstoykov]

If I remember correctly it was too long for me to type out when I was adding it and ... well I haven't renamed it since ;)

[mstoykov]

@imiric it is used in the constant arrival rate executor d093a4a . You can look at the added test for the executor, they show what happens :)

[imiric]

A few minor things and questions. I spotted a few typos, but we can live with those :)

The additional documentation really helps, thanks. Could we link to your gist math paper somewhere as well?

[imiric]

If I'm interpreting this correctly, this lambda will always return false and the break outer on L493 would never be triggered, correct? If so, we could remove the label and check from stripingAlgorithm, and the return from here.

[mstoykov]

This was needed if you wanted to calculate only for one segment in order to short circuit the whole calculation ... if we decide that functionality will never be needed - I will remove it, but I prefer to wait for @na-- to write down what he thinks and then if this is not needed we can drop it.

[imiric]

Consider dropping the Algorithm from the name here. All functions implement "algorithms" :)

Maybe just stripe()?

Also, it could be just me, but I'm not sure I find splitting the implementation into this receiving a lambda is more readable or easier to understand. It essentially moves the "meat" of this algorithm to an external function, which is convenient if this helps with reusing it for other scenarios other than splitting iterations, but it currently dilutes the purpose of this function. Not a big deal, but unless making it generic is a priority, it would be easier to follow if it's embedded as in the original version. After all, we don't gain much by moving it to another place.

[mstoykov]

👍 for dropping Algorithm and stripe sounds even better.

This is for 2 reasons:

1. This is artefact of how it was previously and during the rewrites. As we are running out of time and this is only part of the work needed, and I needed feedback ... in more general sense and there is also
2. While this currently isn't used as I calculate all offsets once and then cache it ... maybe we will want to revert to using the other variant - this will depend both on how much we think that caching actually speedups stuff and what we need in the other executors ... which are both connected.

So I would prefer if we add a TODO to refactor this once we have fixed all the functional issues and probably have gain some experience about what is needed.

[na--]

I haven't fully finished with the code review, still have to fully review some of the trickier implementation details... In general everything mostly LGTM so far, I'm posting the minor nitpicks and comments I've currently got, so you can look through them and either respond or have something to work on while I review the rest.

[na--]

I'm not sure, but I think your current approach may be the better one. My thoughts on this issue:

1. If users just want to run a quickly scaled down version of their test by k6 run --execution-segment=0.1 heavy_test.js, then filling out the sequence with 1/lcd segments would be better. Because in that scenario the segment that the user configured would be larger than or equal to all other 1/lcd segments, its work will be optimally spread.
2. On the other hand, in some cases, calculations would be quicker if we just fill out the sequence with max 2 segments, like how you currently do it. And the work distribution wouldn't be all that terrible, even if the user's segment isn't the biggest one... After all, that's sort of the whole idea 😅
3. If users want to do a quick and dirty distributed execution by specifying --execution-segment without --execution-segment-sequence, then your current approach is also probably better in most cases.

[mstoykov]

it is certainly the most straightforward one to explain as well ... because all the others get hairy with strange segments such as "1/3:2/5" or something like that ...

The biggest problem in my experience is that if you run it with "0:1/3", "1/3:2/3" and "2/3:1". the first and last behave the same while the middle one behaves as I have started to expect (which is not to say that it is wrong).
I don't think it is noticeable if you don't look for it and for the majority of the use cases I would argue this will work just the same :D

[na--]

The biggest problem in my experience is that if you run it with "0:1/3", "1/3:2/3" and "2/3:1". the first and last behave the same while the middle one behaves as I have started to expect (which is not to say that it is wrong).

But that's the thing - if you fill in the sequence with 1/lcd pieces, you'd get this nasty behavior pretty much all the time - equally sized segments would behave absolutely identically. So your current solution would be a better fit for them, I think... In any case, you probably should delete the TODO from here, to avoid confusion - we're not going to change the current implementation without some very good reason.

[na--]

These probably can be pre-calculated in the constructor, given how many times they are being used below? Something for the follow-up optimization pass perhas?

[mstoykov]

I would prefer if we can merge this before I need to rebase it again :D

[na--]

if by "preallocation" you mean "getting them from the buffer pool in the execution state", then that can't happen here. It needs to be in Run(), because the Init() function of all executors is called before they all start, which, because of the startTime option might not be immediately. Please delete the TODO if that was the intention, it has a typo anyway 😝

[mstoykov]

hm ... I forgot about the startTime

[na--]

For a future PR, but now this arrivalRate would be used only for the status text, not for the actual execution, so it's becoming a bit confusing... Please add something like a //TODO: refactor and simplify here, since something like this looks more and more appealing for all of the executors: https://github.com/loadimpact/k6/blob/dc46c03ce1a12df30dba8c5b8d103ebc587aaf5e/lib/executor/externally_controlled.go#L366-L369

[mstoykov]

Yeah I also noticed that now there are like 5 variables which are only used for the progressbar ... But definitely another PR at a later stage

[na--]

I think we can call this "good enough for now", leave the TODOs and refactor it at some later point. We can figure out better names, code structure, and maybe further optimizations once everything else is working, so please add a new low prio + new executors + refactor issue for this.

[mstoykov]

#1379

I wrote it about the ExecutionTuple as sortInterfaceWrapper, in reality, will probably get removed when we next refactor it and lib.ExecutionTuple is a public type :D

[imiric]

Sorry for the delay, LGTM.

I can't comment much on the functionality, the math is a bit overwhelming, but as long as it works and it's an improvement we can always refine it later. Nice work!