-
Notifications
You must be signed in to change notification settings - Fork 8
Implementation of distribution_t data structure #40
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.
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
base: project-sshmidt
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Svetlana!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Svetlana! There is an uninitialized pointer in distribution_clone
, all the rest is just nit-picks ;)
src/daemon/distribution.c
Outdated
}; | ||
for (size_t i = 1; i < num_buckets; i++) { | ||
bucket_array[i].bucket_counter = 0; | ||
bucket_array[i].minimum = bucket_array[i - 1].maximum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At your option: You could use a compound literal here, too:
bucket_array[i] = (bucket_t) {
.minimum = bucket_array[i - 1].maximum,
.maximum = (i == num_buckets-1) ? INFINITY : bucket_array[i - 1].maximum * factor,
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented such approach and realised that now my constructor functions are very similar. The main difference is in calculating maximum boundaries of buckets. Maybe, it would be better to select maximum boundaries first and then call the function that will fill other fields? Or such code duplication is okay?
…ubles equality check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, Svetlana! 😄
src/daemon/distribution_benchmark.c
Outdated
} | ||
|
||
int main() { | ||
FILE *fout = fopen("benchmark.csv", "w"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fopen could possibly not succeed, so I'd recommend to check if fout is equal to NULL and if yes, return.
For consideration:
There is also another possibility to redirect output to file. If you would use only printf, then running ./distribution_benchmark > benchmark.csv should return the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Barbara, for example I wrote to the csv file using printf and writing:
./distribution_benchmark $i >> results.csv
in the bash script.
src/daemon/distribution_benchmark.c
Outdated
FILE *fout = fopen("benchmark.csv", "w"); | ||
fprintf(fout, "Number of buckets,Average for update,Average for percentile,Total for %lu mixed iterations\n", MIXED); | ||
for (size_t num_buckets = 20; num_buckets <= 4000; num_buckets += 20) { | ||
distribution_t *dist = build(num_buckets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend to check if dist is NULL and do some things if yes
static double *linear_upper_bounds(size_t num, double size) { | ||
double *linear_upper_bounds = calloc(num, sizeof(*linear_upper_bounds)); | ||
for (size_t i = 0; i + 1 < num; i++) | ||
linear_upper_bounds[i] = (i + 1) * size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could possibly return an error because linear_update_bounds could be NULL. I'd recommend to check if linear_upper_bounds is NULL and do sth if yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same in the other upper bounds functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your algorithm using segment tree is really interesting Svetlana, great work!
src/daemon/distribution_benchmark.c
Outdated
} | ||
|
||
int main() { | ||
FILE *fout = fopen("benchmark.csv", "w"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Barbara, for example I wrote to the csv file using printf and writing:
./distribution_benchmark $i >> results.csv
in the bash script.
…-interns-2020 branch
Add distribution.h and distribution.c files that contains implementation of distribution_t data structure using the segment-tree approach.
You can read about the functionality of the distribution_t and the segment-tree approach in the design document: https://docs.google.com/document/d/1ccsg5ffUfqt9-mBDGTymRn8X-9Wk1CuGYeMlRxmxiok/edit?usp=sharing