Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
ibessonov commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044510876
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
+
+// Put it within reasonable bounds just in case.
Review Comment:
Too afraid of some weird artifacts. I think that 0.9 upper bound can be
ignored, but the lower bound is mandatory, otherwise we'll get huge stalls on
malfunctioning or otherwise strangely behaving storages (that often result in
huge fsync durations)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
ibessonov commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044458432
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
+
+// Put it within reasonable bounds just in case.
Review Comment:
I don't want any of these values to be too close to zero
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
ibessonov commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044510876
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
+
+// Put it within reasonable bounds just in case.
Review Comment:
Too afraid of some weird artifacts. I think that 0.9 upper bound can be
ignored, but the lower bound is mandatory, otherwise we'll get huge stalls on
malfunctioning or otherwise strangely behaving storages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
ibessonov merged PR #5637: URL: https://github.com/apache/ignite-3/pull/5637 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
sashapolo commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044411169
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -154,10 +161,9 @@ private long computeParkTime(int cpWrittenPages, long
curNanoTime) {
* @return Estimation of work done (in pages).
*/
private int cpDonePagesEstimation(int cpWrittenPages) {
-// TODO: IGNITE-16879 - this only works correctly if time-to-write a
page is close to time-to-sync a page.
-// In reality, this does not seem to hold, which produces wrong
estimations. We could measure the real times
-// in Checkpointer and make this estimation a lot more precise.
-return (cpWrittenPages + cpSyncedPages()) / 2;
+double coefficient = writeVsFsyncCoefficient;
+
+return (int) (cpWrittenPages * coefficient + cpSyncedPages() * (1 -
coefficient));
Review Comment:
You need to update the javadoc
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
+
+// Put it within reasonable bounds just in case.
Review Comment:
What do you mean by "reasonable" bounds?
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
Review Comment:
What's this about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
sashapolo commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044494732
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
+
+// Put it within reasonable bounds just in case.
Review Comment:
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
ibessonov commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044461352
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,35 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+if (progress.currentCheckpointPagesCount() == 0) {
+return;
+}
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
+if (isNaN(coefficient)) {
+return;
+}
+
+double newCoefficient = writeVsFsyncCoefficient * 0.85 + coefficient *
0.15;
Review Comment:
That's an exponential smoothing of the coefficient
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-16879 Add dynamic estimation of write vs fsync duration in throttling [ignite-3]
Copilot commented on code in PR #5637:
URL: https://github.com/apache/ignite-3/pull/5637#discussion_r2044100543
##
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/throttling/SpeedBasedMemoryConsumptionThrottlingStrategy.java:
##
@@ -496,5 +506,31 @@ void reset() {
void finish() {
cpWriteSpeed.closeInterval();
threadIds.clear();
+
+updateWriteVsFsyncCoefficient();
+}
+
+private void updateWriteVsFsyncCoefficient() {
+CheckpointProgress progress = cpProgress.get();
+assert progress != null;
+
+long pagesWriteTimeMillis = progress.getPagesWriteTimeMillis();
+long fsyncTimeMillis = progress.getFsyncTimeMillis();
+
+double coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
Review Comment:
Consider handling the scenario where both pagesWriteTimeMillis and
fsyncTimeMillis are 0 to avoid a 0/0 division, which results in NaN. Adding a
conditional check or default value would improve robustness.
```suggestion
double coefficient;
if (pagesWriteTimeMillis == 0 && fsyncTimeMillis == 0) {
coefficient = 0.5; // Default value when both times are zero
} else {
coefficient = ((double) pagesWriteTimeMillis) /
(pagesWriteTimeMillis + fsyncTimeMillis);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
