Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Yield while building large results in Alternator - rjson::print, executor::batch_get_item #13689

Closed
nuivall opened this issue Apr 27, 2023 · 3 comments · Fixed by #12351
Closed

Yield while building large results in Alternator - rjson::print, executor::batch_get_item #13689

nuivall opened this issue Apr 27, 2023 · 3 comments · Fixed by #12351
Assignees
@nuivall
Labels
area/alternator Alternator related Issues symptom/latency
Milestone
5.4

Comments

@nuivall
Copy link
Member

nuivall commented Apr 27, 2023

Branched from #7926 to separate the issue to smaller chunks.

In this issue we fix (the most obvious cases in which yielding is necessary):

  • The low-level JSON printing code (rjson::print() called in server.cc).
  • For BatchGetItem, executor::batch_get_item() also ends with a loop over all items, which doesn't yield.
@nuivall nuivall added area/alternator Alternator related Issues symptom/latency labels Apr 27, 2023
@nuivall nuivall self-assigned this Apr 27, 2023
@nuivall nuivall mentioned this issue Apr 27, 2023
Merged
@mykaul mykaul added this to the 5.3 milestone Apr 27, 2023
@nyh nyh mentioned this issue Jul 13, 2023
Closed
@nyh
Copy link
Contributor

nyh commented Jul 13, 2023

I'll backport this fix, as it is a bug fix in latency.

nyh added a commit to nyh/scylla that referenced this issue Jul 13, 2023
@nyh
Merge 'Yield while building large results in Alternator - rjson::prin…
9db5364 
…t, executor::batch_get_item' from Marcin Maliszkiewicz

Adds preemption points used in Alternator when:
 - sending bigger json response
 - building results for BatchGetItem

I've tested manually by inserting in preemptible sections (e.g. before `os.write`) code similar to:

    auto start  = std::chrono::steady_clock::now();
    do { } while ((std::chrono::steady_clock::now() - start) < 100ms);

and seeing reactor stall times. After the patch they
were not increasing while before they kept building up due to no preemption.

Refs scylladb#7926
Fixes scylladb#13689

Closes scylladb#12351

* github.com:scylladb/scylladb:
  alternator: remove redundant flush call in make_streamed
  utils: yield when streaming json in print()
  alternator: yield during BatchGetItem operation

(cherry picked from commit d2e0897)
@nyh nyh mentioned this issue Jul 13, 2023
Closed
nyh added a commit that referenced this issue Jul 13, 2023
@nyh
Merge 'Yield while building large results in Alternator - rjson::prin…
bdb93af 
…t, executor::batch_get_item' from Marcin Maliszkiewicz

Adds preemption points used in Alternator when:
 - sending bigger json response
 - building results for BatchGetItem

I've tested manually by inserting in preemptible sections (e.g. before `os.write`) code similar to:

    auto start  = std::chrono::steady_clock::now();
    do { } while ((std::chrono::steady_clock::now() - start) < 100ms);

and seeing reactor stall times. After the patch they
were not increasing while before they kept building up due to no preemption.

Refs #7926
Fixes #13689

Closes #12351

* github.com:scylladb/scylladb:
  alternator: remove redundant flush call in make_streamed
  utils: yield when streaming json in print()
  alternator: yield during BatchGetItem operation

(cherry picked from commit d2e0897)
@nyh
Copy link
Contributor

nyh commented Jul 13, 2023 •
edited

Backported to next-5.3 (bdb93af), next-5.2 (ee8b261), next-5.1 (7d11a34)

nyh added a commit that referenced this issue Jul 13, 2023
@nyh
Merge 'Yield while building large results in Alternator - rjson::prin…
ee8b261 
…t, executor::batch_get_item' from Marcin Maliszkiewicz

Adds preemption points used in Alternator when:
 - sending bigger json response
 - building results for BatchGetItem

I've tested manually by inserting in preemptible sections (e.g. before `os.write`) code similar to:

    auto start  = std::chrono::steady_clock::now();
    do { } while ((std::chrono::steady_clock::now() - start) < 100ms);

and seeing reactor stall times. After the patch they
were not increasing while before they kept building up due to no preemption.

Refs #7926
Fixes #13689

Closes #12351

* github.com:scylladb/scylladb:
  alternator: remove redundant flush call in make_streamed
  utils: yield when streaming json in print()
  alternator: yield during BatchGetItem operation

(cherry picked from commit d2e0897)
nyh added a commit that referenced this issue Jul 13, 2023
@nyh
Merge 'Yield while building large results in Alternator - rjson::prin…
7d11a34 
…t, executor::batch_get_item' from Marcin Maliszkiewicz

Adds preemption points used in Alternator when:
 - sending bigger json response
 - building results for BatchGetItem

I've tested manually by inserting in preemptible sections (e.g. before `os.write`) code similar to:

    auto start  = std::chrono::steady_clock::now();
    do { } while ((std::chrono::steady_clock::now() - start) < 100ms);

and seeing reactor stall times. After the patch they
were not increasing while before they kept building up due to no preemption.

Refs #7926
Fixes #13689

Closes #12351

* github.com:scylladb/scylladb:
  alternator: remove redundant flush call in make_streamed
  utils: yield when streaming json in print()
  alternator: yield during BatchGetItem operation

(cherry picked from commit d2e0897)
@nyh
Copy link
Contributor

nyh commented Jul 16, 2023

Verified that all three backports (to 5.3, 5.2, 5.1) were successfully promoted.

@DoronArazii DoronArazii modified the milestones: 5.3, 5.4 Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nuivall nuivall

Labels
area/alternator Alternator related Issues symptom/latency
Projects
None yet
Milestone
5.4
5 participants
@nyh @nuivall @mykaul @scylladb-promoter @DoronArazii