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

Ncham/duration #31

Open

andrewrch wants to merge 1 commit into amzn:master from andrewrch:ncham/duration-id

andrewrch commented Jun 8, 2018 •

edited

Loading

Issue #, if available: #5

Description of changes:

Made changes to bag to allow storing objects. This is a bit ugly in places because now bag returns a pointer to the things it's stored - in most cases bag is used to store a pointer, so this means casting void* to ptr** and dereferencing again. I was going to maybe write some macros similar to e.g. HT_EVENT to make this less ugly but wanted some feedback before doing that.
Added duration feature. Borrowed (copied) from feature callstack. I currently use a bag to store the events - I'll need to do some more work to properly handle String and Int events. At the moment I just use String duration events for testing. Again, I just wanted to make sure I was on the right track before doing all this. I'd probably just add another bag for int events.
Added an example file which basically just adds 10 events with a bit of sleeping then stops them all.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

andrewrch commented

View reviewed changes

lib/feature_duration.c

+                  return f->bag.size - 1;
+              }
+              HT_DurationNs ht_feature_duration_stop(HT_Timeline* timeline, HT_DurationId id)

Author

andrewrch Jun 8, 2018

Not sure if you want this to return duration?

Member

loganek Jun 8, 2018

I think it's very handy to have this duration as a return value

andrewrch force-pushed the ncham/duration-id branch 5 times, most recently from 4d02933 to 2e4da2c Compare

June 8, 2018 09:25


          [lib] Adding Duration feature, change ht_bag to store objects not poi…

2e4da2c

…nters

loganek reviewed

View reviewed changes

Member

loganek left a comment

In general, the PR looks great. Thank you so much for your work! I think we might need more tests for the duration feature, and changes for the bag. Please let me know if you have time to do this; if not, I'll write those tests. Also, looks like there's some memory problem (see travis output) - let me know if you want to look into this.
Before I merge it, I'd like to do some performance tests for HT_Bag and everything that uses this structure - looks like memcpy and memcmp might be quite slow. If it's too slow, we might consider keeping two versions of bag - for pointers, and for everything else. Unfortunately we don't have too many benchmarks yet, so I'll have to add them.

examples/tutorials/simple_duration/simple_duration.cpp

		@@ -0,0 +1,43 @@
		#include <hawktracer.h>
		#include <unistd.h>

Member

loganek Jun 8, 2018

I guess you need it for sleep()? I'd rather avoid it as it will cause build failure on windows. For C++ files we do use C++11 standard (especially for examples/ and tests/) so feel free to use chrono & thread

Author

andrewrch Jun 9, 2018

:) sure I didn't actually notice this was c++... I'll change that.

examples/tutorials/simple_duration/simple_duration.cpp

+              {
+                  ht_init(argc, argv);
+                  HT_FileDumpListener* file_dump_listener = ht_file_dump_listener_create("test_output.htdump", 4096u, NULL);

Member

loganek Jun 8, 2018

Could the output file name contain an example name?

examples/tutorials/simple_duration/simple_duration.cpp

+                      ht_timeline_register_listener(ht_global_timeline_get(), ht_file_dump_listener_callback, file_dump_listener);
+                  }
+                  ht_feature_duration_enable(ht_global_timeline_get());

Member

loganek Jun 8, 2018

The function might fail. I know it's very unlikely, and the error code will be mostly ignored in that case, but since it's an example, I'd prefer to capture all possible errors here.

Member

loganek Jun 8, 2018

Also, I think we might actually enable this feature in global timeline by default (see global_timeline.cpp file, we already enable e.g. callstack feature)

examples/tutorials/simple_duration/simple_duration.cpp

+                  HT_DurationId ids[NUM_MEASUREMENTS];
+                  char names[NUM_MEASUREMENTS][128];
+                  printf ("Gathering %d measurements\n", NUM_MEASUREMENTS);

Member

loganek Jun 8, 2018

we don't put spaces between function name and (

examples/tutorials/simple_duration/simple_duration.cpp

+                  printf ("Gathering %d measurements\n", NUM_MEASUREMENTS);
+                  for (int i = 0; i < NUM_MEASUREMENTS; i++)
+                  {
+                      sprintf(names[i], "Measurement_name_%d", i);

Member

loganek Jun 8, 2018

Could you use snprintf? I know it's safe in this case, but I'm afraid some of the static code analysis tools might complain about using unsafe function

lib/feature_duration.c

+                  return f->bag.size - 1;
+              }
+              HT_DurationNs ht_feature_duration_stop(HT_Timeline* timeline, HT_DurationId id)

Member

loganek Jun 8, 2018

I think it's very handy to have this duration as a return value

lib/include/hawktracer/feature_duration.h


		#define HT_FEATURE_DURATION 2

		HT_API HT_ErrorCode ht_feature_duration_enable(HT_Timeline* timeline);

Member

loganek Jun 8, 2018

I know that currently we don't have documentation for most of the API, but we try add it at least to a new code. So I'd be very grateful if you could add the documentation here :)

lib/feature_duration.c

+              HT_DurationNs ht_feature_duration_stop(HT_Timeline* timeline, HT_DurationId id)
+              {
+                  HT_FeatureDuration* f = HT_TIMELINE_FEATURE(timeline, HT_FEATURE_DURATION, HT_FeatureDuration);

Member

loganek Jun 8, 2018

I know you've copied it from callstack feature, and I'm ok with merging it as it is. But I think it'd be better to not generate any event here, but just return the value, and it's caller's responsibility to create an event. Current implementation is quite convenient, but from the other hand it always generates an event which is a bit problematic because:

we might not want to have the event
we can only have 2 types events (int duration and string duration) and it's impossible to extend it without modifying the library.

Maybe we could have base functionality which doesn't generate any event, but just returns the duration, and on top of it, build a feature which generates actual events? Then it'd be easy for user to extend it. Ofc. we have the same problem in callstack feature.
As I said, we can leave it as it is for now, and discuss how to make those features better in the future (ideally, before API lock).

Author

andrewrch Jun 9, 2018

Sure, I just copied this behaviour but see your point. If we don't create the event here then I can simplify this PR quite a lot since i'd only need to store a timestamp in the bag.

lib/bag.c

                   bag->capacity = 0;
                   bag->size = 0;
               }
+              void*
+              ht_bag_get_n(HT_Bag* bag, size_t n)

Member

loganek Jun 8, 2018

shouldn't it be ht_bag_get_nth?

examples/tutorials/CMakeLists.txt

		@@ -1 +1,2 @@
		add_subdirectory(hello_world)
		add_subdirectory(simple_duration)

Member

loganek Jun 8, 2018

I'm not sure if tutorials/ is the best place for it, as we don't have article in the documentation for this feature. You could move it to examples/duration_feature, and once we have an article for it, we can move it back to tutorials

Author

andrewrch commented Jun 9, 2018

Thanks for the feedback. I'll try to update the PR in a day or two 👍

Member

loganek commented Jun 16, 2018

Would it be possible to add 'thread-safe' option?

loganek force-pushed the master branch from 247552d to 67169ec Compare

July 15, 2018 00:56

loganek force-pushed the master branch 2 times, most recently from ce6f7d8 to 3ca6711 Compare

January 19, 2020 22:16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet