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
add microsecond precise timers for measuring various intervals #2495
Conversation
da3f93c
to
cd37983
Compare
|
cd37983
to
941d7bb
Compare
Done. |
Any other blockers? |
The CI builds are still red so I haven't really checked out the details.
|
ada1661
to
558174d
Compare
Saw some errors, yeah, but I thought they were pre-existing. But you're right, the above was introduced by my changes. Fixing it now. |
558174d
to
ab269cb
Compare
Also failed the symbols-in-versions test (1119). Fixed that. |
ab269cb
to
6ebc305
Compare
@bagder Does Travis build each commit in a PR separately or just the last one? I fixed the issue that you pointed out above, then it built but failed elsewhere (with the missing versioning of the new symbols) so I fixed that... now it's back to failing at the above place. Weird. |
eb924bf
to
924da0b
Compare
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 think this looks great, I only have a few comments and questions to ponder.
docs/examples/progressfunc.c
Outdated
|
||
/* under certain circumstances it may be desirable for certain functionality | ||
to only run every N seconds, in order to do this the transaction time can | ||
be used */ | ||
if((curtime - myp->lastruntime) >= MINIMAL_PROGRESS_FUNCTIONALITY_INTERVAL) { | ||
if(((curtime - myp->lastruntime) / 1000000) >= MINIMAL_PROGRESS_FUNCTIONALITY_INTERVAL) { |
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.
Wouldn't this look nicer if instead the constant MINIMAL_PROGRESS_FUNCTIONALITY_INTERVAL
was multiplied with 1000000 ?
lib/getinfo.c
Outdated
@@ -281,6 +281,30 @@ static CURLcode getinfo_offt(struct Curl_easy *data, CURLINFO info, | |||
*param_offt = (data->progress.flags & PGRS_UL_SIZE_KNOWN)? | |||
data->progress.size_ul:-1; | |||
break; | |||
#if CURL_SIZEOF_CURL_OFF_T == 8 |
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.
Why limit these to 64bit only?
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.
Okay, since it's a relative timestamp and not absolute, sure, I'll make it 32-bit as well.
include/curl/curl.h
Outdated
((off_t) / 1000000L), (long)((off_t) % 1000000L) | ||
|
||
#define CURL_FORMAT_TIME_TUPLE \ | ||
CURL_FORMAT_CURL_OFF_T ".%ld" |
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.
These might be handy for an application, but they're also very basic and for very specific and narrow use cases. Does it really add anything to provide them in the public header?
To me they feel more like handy macros for our test suite.
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.
Well, since other format specifiers were exposed in system.h
I was trying to follow that model.
curl_easy_getinfo(curl, CURLINFO_CONNECT_TIME, &time_connect); | ||
curl_easy_getinfo(curl, CURLINFO_PRETRANSFER_TIME, &time_pretransfer); | ||
curl_easy_getinfo(curl, CURLINFO_STARTTRANSFER_TIME, | ||
curl_easy_getinfo(curl, CURLINFO_NAMELOOKUP_TIME_T, &time_namelookup); |
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.
Since you limited the code to extract this time to 64 bit only, won't this code fail hard on 32 bit systems?
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.
Made it 32-bit also.
docs/libcurl/symbols-in-versions
Outdated
@@ -206,9 +206,11 @@ CURLHEADER_SEPARATE 7.37.0 | |||
CURLHEADER_UNIFIED 7.37.0 | |||
CURLINFO_ACTIVESOCKET 7.45.0 | |||
CURLINFO_APPCONNECT_TIME 7.19.0 | |||
CURLINFO_APPCONNECT_TIME_T 7.60.0 |
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.
Since the feature window has closed now for 7.60.0 and this is a change, we can assume this change can land in 7.61.0.
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.
Changed... the Travis testing is kicking my ass, though... Can't get it to pass the tests 1139 and 1140.
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 don't see it in the most recent commit. I see it fails because of this:
lib500.c: In function ‘test’:
lib500.c:115:112: error: expected identifier before ‘%’ token
fprintf(moo, "namelookup vs connect: %" CURL_FORMAT_CURL_OFF_T ".%06ld " %" CURL_FORMAT_CURL_OFF_T ".%06ld\n",
there's a quote in middle of the ".%06ld " %"
which it looks like causes a slew of syntax errors
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.
Fixed
5f0e463
to
678268f
Compare
Not understanding what's needed here:
|
because it's not documented, you need man pages for each. how is CURLINFO_APPCONNECT_TIME different from CURLINFO_APPCONNECT_TIME_T |
Can we have the *TIME and *TIME_T versions share the same pages? |
f5e2890
to
d33b20f
Compare
5d33f05
to
62a1d5a
Compare
We overload the curl_off_t, if it's 64-bits wide, for extending the API to allow retrieving times in microseconds (which is how they're stored internally).
62a1d5a
to
450fc14
Compare
I'll check it again it looks like you did more work on it. Even if approved it can't be added until the next feature window, in 2 weeks. |
When does the window reopen? |
At release, which was yesterday! |
Thanks! |
Work in progress.
Trying to make the definition of
CURLINFO_TOTAL_TIME_T
, et al be conditional on whetherCURL_SIZEOF_CURL_OFF_T == 8
or not.CURL_SIZEOF_CURL_OFF_T
is defined inlib/curl_setup.h
but that's not included incurl/curl.h
... investigating a work-around.