/curl

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Reviewers

@bagder bagder

@jay jay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@pprindeville @bagder @jay
@pprindeville
Contributor

pprindeville commented Apr 14, 2018

Work in progress.

Trying to make the definition of CURLINFO_TOTAL_TIME_T, et al be conditional on whether CURL_SIZEOF_CURL_OFF_T == 8 or not.

CURL_SIZEOF_CURL_OFF_T is defined in lib/curl_setup.h but that's not included in curl/curl.h... investigating a work-around.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 16, 2018

Member 
../../include/curl/curl.h:2530:5: warning: "CURL_SIZEOF_CURL_OFF_T" is not defined [-Wundef]
 #if CURL_SIZEOF_CURL_OFF_T == 8
     ^

CURL_SIZEOF_CURL_OFF_T is not defined by the global public curl headers so you can't rely on that in there! But that I don't think you should add ifdef-conditional options to begin with so just remove the ifdef...
Member

bagder commented Apr 16, 2018 

../../include/curl/curl.h:2530:5: warning: "CURL_SIZEOF_CURL_OFF_T" is not defined [-Wundef]
 #if CURL_SIZEOF_CURL_OFF_T == 8
     ^

CURL_SIZEOF_CURL_OFF_T is not defined by the global public curl headers so you can't rely on that in there! But that I don't think you should add ifdef-conditional options to begin with so just remove the ifdef...
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 16, 2018

Contributor

But that I don't think you should add ifdef-conditional options to begin with so just remove the ifdef...

Done.
Contributor

pprindeville commented Apr 16, 2018

But that I don't think you should add ifdef-conditional options to begin with so just remove the ifdef...

Done.
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 19, 2018

Contributor

Any other blockers?
Contributor

pprindeville commented Apr 19, 2018

Any other blockers?
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 19, 2018

Member

The CI builds are still red so I haven't really checked out the details.

progressfunc.c:52:3: error: conversion to ‘double’ from ‘curl_off_t’ may alter its value [-Werror=conversion]
   if((curtime - myp->lastruntime) >= MINIMAL_PROGRESS_FUNCTIONALITY_INTERVAL) {
   ^
progressfunc.c:53:5: error: conversion to ‘double’ from ‘curl_off_t’ may alter its value [-Werror=conversion]
     myp->lastruntime = curtime;
     ^
Member

bagder commented Apr 19, 2018

The CI builds are still red so I haven't really checked out the details.

progressfunc.c:52:3: error: conversion to ‘double’ from ‘curl_off_t’ may alter its value [-Werror=conversion]
   if((curtime - myp->lastruntime) >= MINIMAL_PROGRESS_FUNCTIONALITY_INTERVAL) {
   ^
progressfunc.c:53:5: error: conversion to ‘double’ from ‘curl_off_t’ may alter its value [-Werror=conversion]
     myp->lastruntime = curtime;
     ^
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 19, 2018

Contributor

The CI builds are still red so I haven't really checked out the details.

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.
Contributor

pprindeville commented Apr 19, 2018

The CI builds are still red so I haven't really checked out the details.

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.
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 20, 2018

Contributor

Also failed the symbols-in-versions test (1119). Fixed that.
Contributor

pprindeville commented Apr 20, 2018

Also failed the symbols-in-versions test (1119). Fixed that.
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 20, 2018

Contributor

@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.
Contributor

pprindeville commented Apr 20, 2018

@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.
@bagder

bagder reviewed Apr 20, 2018
View changes

I think this looks great, I only have a few comments and questions to ponder.

Show outdated Hide outdated docs/examples/progressfunc.c
Show outdated Hide outdated lib/getinfo.c
Show outdated Hide outdated include/curl/curl.h
tests/libtest/lib500.c
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);

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2018

Member

Since you limited the code to extract this time to 64 bit only, won't this code fail hard on 32 bit systems?
Copy link
@bagder

bagder Apr 20, 2018

Member

Since you limited the code to extract this time to 64 bit only, won't this code fail hard on 32 bit systems?

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 20, 2018

Contributor

Made it 32-bit also.
Copy link
@pprindeville

pprindeville Apr 20, 2018

Contributor

Made it 32-bit also.
Show outdated Hide outdated docs/libcurl/symbols-in-versions
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 21, 2018

Contributor

Not understanding what's needed here:

test 1139...[Verify that all libcurl options have man pages]

perl  returned 7, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1139
=== Start of file stderr1139
 Missing CURLINFO_APPCONNECT_TIME_T.3
 Missing CURLINFO_CONNECT_TIME_T.3
 Missing CURLINFO_NAMELOOKUP_TIME_T.3
 Missing CURLINFO_PRETRANSFER_TIME_T.3
 Missing CURLINFO_REDIRECT_TIME_T.3
 Missing CURLINFO_STARTTRANSFER_TIME_T.3
 Missing CURLINFO_TOTAL_TIME_T.3
=== End of file stderr1139
test 1140...[Verify the nroff of man pages]

perl  returned 7, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1140
=== Start of file stderr1140
 error: ./../docs/libcurl/curl_easy_getinfo.3:65: referring to non-existing man page CURLINFO_TOTAL_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:71: referring to non-existing man page CURLINFO_NAMELOOKUP_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:77: referring to non-existing man page CURLINFO_CONNECT_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:83: referring to non-existing man page CURLINFO_APPCONNECT_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:89: referring to non-existing man page CURLINFO_PRETRANSFER_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:95: referring to non-existing man page CURLINFO_STARTTRANSFER_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:101: referring to non-existing man page CURLINFO_REDIRECT_TIME_T.3
=== End of file stderr1140
Contributor

pprindeville commented Apr 21, 2018

Not understanding what's needed here:

test 1139...[Verify that all libcurl options have man pages]

perl  returned 7, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1139
=== Start of file stderr1139
 Missing CURLINFO_APPCONNECT_TIME_T.3
 Missing CURLINFO_CONNECT_TIME_T.3
 Missing CURLINFO_NAMELOOKUP_TIME_T.3
 Missing CURLINFO_PRETRANSFER_TIME_T.3
 Missing CURLINFO_REDIRECT_TIME_T.3
 Missing CURLINFO_STARTTRANSFER_TIME_T.3
 Missing CURLINFO_TOTAL_TIME_T.3
=== End of file stderr1139
test 1140...[Verify the nroff of man pages]

perl  returned 7, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1140
=== Start of file stderr1140
 error: ./../docs/libcurl/curl_easy_getinfo.3:65: referring to non-existing man page CURLINFO_TOTAL_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:71: referring to non-existing man page CURLINFO_NAMELOOKUP_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:77: referring to non-existing man page CURLINFO_CONNECT_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:83: referring to non-existing man page CURLINFO_APPCONNECT_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:89: referring to non-existing man page CURLINFO_PRETRANSFER_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:95: referring to non-existing man page CURLINFO_STARTTRANSFER_TIME_T.3
 error: ./../docs/libcurl/curl_easy_getinfo.3:101: referring to non-existing man page CURLINFO_REDIRECT_TIME_T.3
=== End of file stderr1140
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Apr 21, 2018

Member

because it's not documented, you need man pages for each. how is CURLINFO_APPCONNECT_TIME different from CURLINFO_APPCONNECT_TIME_T
Member

jay commented Apr 21, 2018

because it's not documented, you need man pages for each. how is CURLINFO_APPCONNECT_TIME different from CURLINFO_APPCONNECT_TIME_T
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville Apr 21, 2018

Contributor

Can we have the *TIME and *TIME_T versions share the same pages?
Contributor

pprindeville commented Apr 21, 2018

Can we have the *TIME and *TIME_T versions share the same pages?
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville May 1, 2018

Contributor

I think the PR is good to go. @jay @bagder?
Contributor

pprindeville commented May 1, 2018

I think the PR is good to go. @jay @bagder?
@pprindeville
add microsecond precise timers for measuring various intervals 
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).
Loading status checks…
450fc14
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 2, 2018

Member

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.
Member

jay commented May 2, 2018

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.
@pprindeville

This comment has been minimized.

Show comment
Hide comment
@pprindeville

pprindeville May 16, 2018

Contributor

When does the window reopen?
Contributor

pprindeville commented May 16, 2018

When does the window reopen?
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 17, 2018

Member

At release, which was yesterday!
Member

bagder commented May 17, 2018

At release, which was yesterday!
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 17, 2018

Member

Thanks!
Member

bagder commented May 17, 2018

Thanks!

@bagder bagder closed this in ce2140a May 17, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2018

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