Use a single segmented index for local and global iterations #2057

mstoykov · 2021-06-10T13:20:17Z

No description provided.

This removes the need for a lot of the additional synchronization. This also makes it so there are global iterations for all executors. For some of those it means that the global iteration will be *unique* among instances but doesn't necessary mean that iterations with smaller numbers *have* run or *will* run in the same test run. This is somewhat like how it is for the executors for which we already had it, there is nothing making certain that a shared-iterations executor will not run out of time to run all the iterations on one of the instances or a arrival-rate to not have to drop iterations due to lack VUs to execute them.

imiric

Thanks @mstoykov, this is indeed simpler! 👏

I thought about using a single function for both iterations, but didn't consider using SegmentedIndex for it. I suppose that has a slight overhead since before it was a single *uint64 and atomic and now it's more complex, but since we're doing it for all executors it shouldn't matter.

Just left some minor comments, but LGTM. 👍

imiric · 2021-06-10T14:53:51Z

js/init_and_modules_test.go

 	"go.k6.io/k6/js"
 	"go.k6.io/k6/js/common"
 	"go.k6.io/k6/js/modules"
 	"go.k6.io/k6/lib"
 	"go.k6.io/k6/lib/testutils"
 	"go.k6.io/k6/loader"
 	"go.k6.io/k6/stats"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"gopkg.in/guregu/null.v3"


Nit and unrelated to this PR, but this is still weirdly formatted. Did you add go.k6.io/k6 to goimports -local?

no I haven't ... and I am against it as the whole idea of gofmt (and goimports respectively) is for them to work the same for everybody and to preferably have 0 knobs ...

Even if I decide to go through the trouble of doing this is everyone who contributes will likely not do it so we will need to ask them to ... fix it, which IMO is a waste of everybodies time, as again you probably shouldn't go look at 20 lines of imports and care how they are ordered.

Don't get me wrong if goimprots decides to default to first putting the names of the module you are in 👍 , but me telling it that it should do it is a totally different thing.

I am against it as the whole idea of gofmt (and goimports respectively) is for them to work the same for everybody and to preferably have 0 knobs

Yeah, but they don't 😄

I don't think we should insist on this for external contributors, but we at least can have that configured so that whenever we change a file it's updated. It's a minor thing, but increases readability of imports substantially IMO.

I don't think we should insist on this for external contributors, but we at least can have that configured so that whenever we change a file it's updated.

And apparently, if it's not configured and someone contributes it will move them ... so this IMO should just not be a thing

lib/executor/base_executor.go

imiric · 2021-06-10T15:08:22Z

lib/executor/ramping_arrival_rate.go

@@ -189,21 +188,11 @@ func (varr *RampingArrivalRate) Init(ctx context.Context) error {
 	et, err := varr.BaseExecutor.executionState.ExecutionTuple.GetNewExecutionTupleFromValue(varr.config.MaxVUs.Int64)
 	varr.et = et
 	start, offsets, lcd := et.GetStripedOffsets()
-	varr.segIdx = lib.NewSegmentedIndex(start, lcd, offsets)
+	varr.iterationSegIndex = lib.NewSegmentedIndex(start, lcd, offsets)


Hhmm do we still need to redefine it here? Couldn't we use the instance created in NewBaseExecutor(), in which case we can get rid of this Init() entirely?

I do think it is needed as this is based on the execution tuple created from the MaxVUs not just based on the execution segment alone.

If I remember correctly this meant that for some corner cases there will be ... wrong values returned otherwise:
execution segment 0:1/2,1/2:1 with 3 VUs will mean that 1 of the instances will have 2 VUs and there other 1 VU so if we are doing arrival rate of 3 the iterations should be

1st instance: 1, 3, 4,6, ... 2nd instance: 2,5,8 ...

but if you just base it on the original execution segments they will be

1st instance: 1,3,5,7 ... 2nd instance: 2,4,6,8 ...

It will still do the same amount of iteration just in the case that there needed to be 12(divides by 2 and 3 ;) )(let's say the rate is 3 and we run for 4 seconds ;) ) iterations at some point there will be ... holes:

1st instance (has to do 2/3 of 12 so 8): 1,3,5,7,9,11,13,15 2nd instance (has to do 1/3 of 12 so 4):,2,4,6,8

P.s. I wrote this quickly so ... likely some of my math is slightly wrong and I also index from 1 ;)

Yeah, because of differences like that, I'd prefer if we don't have NextIterationCounters() in BaseExecutor and instead have it as a helper. Maybe something like GetSegmentedCounterIterator(ExecutionTuple) func() (uint64, uint64) that essentially returns the next() closure?

I wanted to do this fast ... so I didn't do it, but IMO SegmentedIndex should have 0 amount of changes from the previous unexported version and it is the nextIterationCounters or w/e will replace it that should have the mutex and get the counters/indexes ...

I can do it in this PR if you want me to ? WDYT @na-- @imiric

I think we should probably have 2 different types - a simple SegmentedIndex without any locking or synchronization, and a SynchronizedSegmentedIndex (or some better name...) that has locking and atomic operations.

I don't think a second type is needed, as we have 1 place we use it and it's only for 1 method it is literally going to be

func nextIteartionsCounters() (uint64, uint64){ someMutex.Lock() defer someMutex.Unlock() segmentedIndex.next() return segmentedIndex.scaled, segmentedIndex.unscaled }

I see no reason to make a whole new type for that .. .but it definitely is better so I am fine with that as well :)

btw I had this pending as a comment in the main PR, but probably worth mentioning here: SegmentedIndexResult seems unnecessary. We are never going to have more than the scaled and unscaled indexes in this type, so a tuple (i.e. 2 return values) seems a bit more idiomatic in Go.

If we ever add more things on top of SegmentedIndex, we are likely to do it as another wrapper, not by changing the original (synchronized or unsynchromized), so we can add a struct then, if we need to.

(didn't see the previous comment when I wrote my last one...)

👍 @mstoykov I'm fine with having the locking only in the helper function where it's needed

na--

This LGTM in principle, though as I said in the inline comment, I think we shouldn't add to BaseExecutor. I also haven't looked too closely at the details, we can fix any of my nitpicks in #1863, since there we can see the full diff with master.

Co-authored-by: Ivan Mirić <ivan@loadimpact.com>

codecov-commenter · 2021-06-11T07:50:20Z

Codecov Report

Merging #2057 (4077201) into feat/1320-execution-api (e9feff3) will decrease coverage by 0.37%.
The diff coverage is 100.00%.

❗ Current head 4077201 differs from pull request most recent head 4675405. Consider uploading reports for the commit 4675405 to get more accurate results

@@                     Coverage Diff                     @@
##           feat/1320-execution-api    #2057      +/-   ##
===========================================================
- Coverage                    72.33%   71.96%   -0.38%     
===========================================================
  Files                          182      178       -4     
  Lines                        14404    14323      -81     
===========================================================
- Hits                         10419    10307     -112     
- Misses                        3350     3363      +13     
- Partials                       635      653      +18

Flag	Coverage Δ
ubuntu	`71.96% <100.00%> (-0.33%)`	⬇️
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/runner.go	`81.91% <100.00%> (-1.85%)`	⬇️
lib/execution_segment.go	`92.01% <100.00%> (-0.17%)`	⬇️
lib/executor/base_executor.go	`85.71% <100.00%> (+3.10%)`	⬆️
lib/executor/constant_arrival_rate.go	`96.49% <100.00%> (-0.09%)`	⬇️
lib/executor/constant_vus.go	`96.42% <100.00%> (-0.05%)`	⬇️
lib/executor/externally_controlled.go	`76.57% <100.00%> (ø)`
lib/executor/helpers.go	`96.51% <100.00%> (-0.08%)`	⬇️
lib/executor/per_vu_iterations.go	`97.19% <100.00%> (-0.03%)`	⬇️
lib/executor/ramping_arrival_rate.go	`96.39% <100.00%> (-0.07%)`	⬇️
lib/executor/ramping_vus.go	`93.36% <100.00%> (-0.03%)`	⬇️
... and 34 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 e9feff3...4675405. Read the comment docs.

imiric · 2021-06-11T08:33:03Z

js/runner.go

-	}
-	if u.getNextScGlobalIter != nil {
-		u.scIterGlobal = u.getNextScGlobalIter()
+	// TODO remove this


I don't think comments like this are helpful without indicating why. ;)

There is nothing stopping 5 VUs starting a shared iterations at the same time and one of them getting "slower" to the actual execution than it's iteration counter suggests.

imiric

LGTM 👏

* Use a single segmented index for local and global iterations This removes the need for a lot of the additional synchronization. This also makes it so there are global iterations for all executors. For some of those it means that the global iteration will be *unique* among instances but doesn't necessary mean that iterations with smaller numbers *have* run or *will* run in the same test run. This is somewhat like how it is for the executors for which we already had it, there is nothing making certain that a shared-iterations executor will not run out of time to run all the iterations on one of the instances or a arrival-rate to not have to drop iterations due to lack VUs to execute them. * Update lib/executor/base_executor.go Co-authored-by: Ivan Mirić <ivan@loadimpact.com> * unexport and comment nextIterationCounters * Remove synchronization in SegmetnedIndex * Drop SegmentedIndexResult * Fix race in TestSharedIterationsGlobalIters There is nothing stopping 5 VUs starting a shared iterations at the same time and one of them getting "slower" to the actual execution than it's iteration counter suggests. Co-authored-by: Ivan Mirić <ivan@loadimpact.com>

mstoykov mentioned this pull request Jun 10, 2021

Core changes for k6/x/execution JS module #1863

Merged

imiric reviewed Jun 10, 2021

View reviewed changes

imiric requested a review from na-- June 10, 2021 15:16

na-- reviewed Jun 11, 2021

View reviewed changes

mstoykov and others added 2 commits June 11, 2021 10:34

Update lib/executor/base_executor.go

39d634d

Co-authored-by: Ivan Mirić <ivan@loadimpact.com>

unexport and comment nextIterationCounters

cfd3f59

Remove synchronization in SegmetnedIndex

2dc5a6a

imiric reviewed Jun 11, 2021

View reviewed changes

mstoykov added 2 commits June 11, 2021 11:42

Drop SegmentedIndexResult

c02986d

Fix race in TestSharedIterationsGlobalIters

4675405

There is nothing stopping 5 VUs starting a shared iterations at the same time and one of them getting "slower" to the actual execution than it's iteration counter suggests.

mstoykov requested review from imiric and na-- June 11, 2021 09:57

imiric approved these changes Jun 11, 2021

View reviewed changes

imiric merged commit f620256 into feat/1320-execution-api Jun 11, 2021

imiric deleted the feat/1320-execution-api-fix branch June 11, 2021 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a single segmented index for local and global iterations #2057

Use a single segmented index for local and global iterations #2057

mstoykov commented Jun 10, 2021

imiric left a comment

imiric Jun 10, 2021

mstoykov Jun 11, 2021

imiric Jun 11, 2021

mstoykov Jun 11, 2021

imiric Jun 10, 2021

mstoykov Jun 10, 2021 •

edited

Loading

na-- Jun 11, 2021 •

edited

Loading

mstoykov Jun 11, 2021

na-- Jun 11, 2021

mstoykov Jun 11, 2021

na-- Jun 11, 2021

na-- Jun 11, 2021

na-- left a comment

codecov-commenter commented Jun 11, 2021 •

edited

Loading

imiric Jun 11, 2021

imiric left a comment

Use a single segmented index for local and global iterations #2057

Use a single segmented index for local and global iterations #2057

Conversation

mstoykov commented Jun 10, 2021

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

na-- Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 11, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

mstoykov Jun 10, 2021 •

edited

Loading

na-- Jun 11, 2021 •

edited

Loading

codecov-commenter commented Jun 11, 2021 •

edited

Loading