Time API #159

gonsie · 2019-08-21T18:42:22Z

This is a new API which allows for user defined tw_stime types. It is implemented through #define macros, which should have minimal performance impact in the nominal case. Existing models do not need to change, unless they plan on supporting various tw_stime types.

See the previous conversation in #157. This PR has develop as the base.

If this merge represents a feature addition to ROSS, the following items must be completed before the branch will be merged:

Document the feature on the blog (See the website Contributing guide).
Include a link to your blog post in the Pull Request.
Builds should cleanly compile with -Wall and -Wextra.
One or more TravisCI tests should be created (and they should pass)
Through the TravisCI tests, coverage should increase
Test with CODES to ensure everything continues to work

- gvt_print_interval (gvt/mpi_allreduce.h) - percent_complete (gvt/mpi_allreduce.h) - g_tw_ts_end - g_tw_lookahead - g_tw_min_detected_offset

3 API operations, implemented in #define - TW_STIME_DBL(x) : convert tw_stime to double - TW_STIME_CMP(x, y) : compare two tw_stime objects - TW_STIME_ADD(x, y) : add two tw_stime objects into a new object - TW_STIME_MAX : return a "max" tw_stime object (similar to DBL_MAX)

For real-time reporting... not based on cycle counts

codecov · 2019-08-22T00:30:15Z

Codecov Report

Merging #159 into develop will decrease coverage by 0.25%.
The diff coverage is 71%.

@@             Coverage Diff             @@
##           develop     #159      +/-   ##
===========================================
- Coverage    58.42%   58.17%   -0.26%     
===========================================
  Files           32       32              
  Lines         3541     3558      +17     
===========================================
+ Hits          2069     2070       +1     
- Misses        1472     1488      +16

Impacted Files	Coverage Δ
core/tw-eventq.h	`91.17% <ø> (ø)`	⬆️
core/instrumentation/st-instrumentation.c	`100% <ø> (ø)`	⬆️
core/tw-setup.c	`70.9% <ø> (ø)`	⬆️
core/ross-inline.h	`40% <ø> (ø)`	⬆️
core/instrumentation/st-model-data.c	`75% <100%> (ø)`	⬆️
core/tw-pe.c	`90% <100%> (ø)`	⬆️
core/tw-kp.c	`79.45% <100%> (ø)`	⬆️
core/instrumentation/st-event-trace.c	`80.76% <100%> (ø)`	⬆️
core/instrumentation/st-sim-engine.c	`98.13% <100%> (ø)`	⬆️
core/gvt/mpi_allreduce.h	`87.5% <100%> (+0.83%)`	⬆️
... and 10 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 995a433...075327e. Read the comment docs.

gonsie · 2019-08-22T21:12:34Z

So I was wrong, this does represent an API change to ROSS model. I've renamed TWOPT_STIME to TWOPT_DOUBLE to be explicit about the accepted values.

This API change means that we'd have do an 8.0 release. Alternatively, we could re-add TWOPT_STIME (as a deprecated option) for a 7.2 release... removing TWOPT_STIME in an eventual 8.0 release.

gonsie · 2019-08-22T21:22:02Z

Since this is an API change, I've created a CODES branch for the update.

gonsie · 2019-08-22T21:23:43Z

There is no functionality change, so no new unit tests are needed. Yet, coverage has increased 😄

Revert "tw-opts: removed STIME opt, replaced with DOUBLE" This reverts commit 61b6ef8.

caitlinross

Just missing a few casts to double where instrumentation sampling functions are called (because of integer division).

core/gvt/mpi_allreduce.c

core/tw-sched.c

caitlinross · 2019-09-14T18:22:58Z

I think once those few changes I pointed out are made, it's good to merge!

caitlinross · 2019-09-14T18:29:52Z

Oh yeah and @gonsie in regards to CODES, it's actually been moved to GitHub so you may want to submit the PR there instead of Gitlab.

gonsie · 2019-09-16T21:33:36Z

thanks @caitlinross! I've updated the PR with your change requests. Please double check that I'm doing the double casting in the way you want. Then merge, or give me the go-ahead.

thanks for the update on codes as well! I'll PR my update to them once we have a 7.2 release.

core/gvt/mpi_allreduce.c

gonsie · 2019-09-18T18:18:08Z

Got it!

For realz this time @caitlinross !

Merge release-7.2.0: * New default clock: get-time-of-day (#170) * STime API (#159) * Add ARMv7l arch support to ROSS (#155) * Fix to generate covage stats (#150) * changing damaris submodule/directory to risa * Updated README * Update to the way the build process grabs version number (#148) * simplifying the build of static or shared libraries (#147) * ROSS cleanup: -Wall, -Wextra, and more (#135)

gonsie added 17 commits July 17, 2019 14:05

whitespace

c1870db

added TWOPT_DOUBLE type

006f199

whitespace

ae4b718

convert some global vars to be explicitly doubles

b7c1ce9

- gvt_print_interval (gvt/mpi_allreduce.h) - percent_complete (gvt/mpi_allreduce.h) - g_tw_ts_end - g_tw_lookahead - g_tw_min_detected_offset

whitespace

8b8621c

convert ROSS over to TW_STIME API

abf3333

whitespace

7c83c0c

tw-opts: removed STIME opt, replaced with DOUBLE

61b6ef8

st_collect functions should use tw_clock (not tw_stime)

f03e6cb

instrumentation global vars should be doubles (not tw_stime)

d8a24c1

explicit use of ternary operator from ROSS_MIN for tw_stime

c19f740

one more instrumentation update

56c4f19

removed ROSS_MIN with some code changes

a894ef4

MPI_TYPE_TW_STIME and TW_STIME_CRT

de095d7

convert phold opts to DOUBLE (not STIME)

3cdfe08

tw_stime should have been changed to double, not tw_clock

f5b9ddb

For real-time reporting... not based on cycle counts

gonsie mentioned this pull request Aug 21, 2019

tw_stime API post ROSS-org/ross-org.github.io#4

Merged

Merge branch 'develop' into time-api

e3a3825

Re-add STIME opt, with deprecation warning

8bb4238

Revert "tw-opts: removed STIME opt, replaced with DOUBLE" This reverts commit 61b6ef8.

caitlinross requested changes Sep 14, 2019

View reviewed changes

core/gvt/mpi_allreduce.c Outdated Show resolved Hide resolved

core/tw-sched.c Outdated Show resolved Hide resolved

core/tw-sched.c Outdated Show resolved Hide resolved

core/tw-sched.c Outdated Show resolved Hide resolved

core/tw-sched.c Outdated Show resolved Hide resolved

explicit double cast for real time value

41fecba

caitlinross requested changes Sep 18, 2019

View reviewed changes

core/gvt/mpi_allreduce.c Outdated Show resolved Hide resolved

proper double conversion

075327e

This was referenced Sep 18, 2019

Support for various tw_stime types #71

Closed

integer tw_stime #58

Closed

support for "long double" tw_stime #34

Closed

caitlinross merged commit 3f21c68 into develop Sep 18, 2019

caitlinross deleted the time-api branch September 18, 2019 23:48

gonsie mentioned this pull request Dec 2, 2019

Release 7.2.0 #171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time API #159

Time API #159

gonsie commented Aug 21, 2019 •

edited

codecov bot commented Aug 22, 2019 •

edited

gonsie commented Aug 22, 2019

gonsie commented Aug 22, 2019

gonsie commented Aug 22, 2019

caitlinross left a comment

caitlinross commented Sep 14, 2019

caitlinross commented Sep 14, 2019

gonsie commented Sep 16, 2019

gonsie commented Sep 18, 2019

Time API #159

Time API #159

Conversation

gonsie commented Aug 21, 2019 • edited

codecov bot commented Aug 22, 2019 • edited

Codecov Report

gonsie commented Aug 22, 2019

gonsie commented Aug 22, 2019

gonsie commented Aug 22, 2019

caitlinross left a comment

Choose a reason for hiding this comment

caitlinross commented Sep 14, 2019

caitlinross commented Sep 14, 2019

gonsie commented Sep 16, 2019

gonsie commented Sep 18, 2019

gonsie commented Aug 21, 2019 •

edited

codecov bot commented Aug 22, 2019 •

edited