Skip to content
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

JDK-8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps #3869

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented May 5, 2021

We have os::iso8601_time(), which gives an ISO8601 timestamp of the current time. It would be very useful to have a second variant which can be fed an arbitrary numerical timestamp.

This is useful in the context of making asynchronous UL logging cheaper (see JDK-8229517)

This patch provides an additional API:
char* os::iso8601_time(jlong milliseconds_since_19700101, char* buffer, size_t buffer_length, bool utc);
alongside the existing
char* os::iso8601_time(char* buffer, size_t buffer_length, bool utc);
and implements the latter using the former. Not much code added.

In addition, it adds a regression gtest for these APIs.

Please ignore the harfbuzz change, its a build fix needed for older gcc, will be removed before final push.

Testing: GHA, manual gtests, SAP nightlies on all our platforms.

Thanks, Thomas


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3869/head:pull/3869
$ git checkout pull/3869

Update a local copy of the PR:
$ git checkout pull/3869
$ git pull https://git.openjdk.java.net/jdk pull/3869/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3869

View PR using the GUI difftool:
$ git pr show -t 3869

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3869.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 5, 2021

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 5, 2021

@tstuefe The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 5, 2021
Copy link
Member

@navyxliu navyxliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API change looks good to me. I understand that this is prerequisite of your logdecoration change. make sense.

I don't understand your change in harfbuzz header.

@@ -375,7 +375,7 @@ hb_iter_with_fallback_t<machine_index_t<Iter>,
typename Iter::item_t>
{
machine_index_t (const Iter& it) : it (it) {}
machine_index_t (const machine_index_t& o) : it (o.it) {}
machine_index_t (const machine_index_t& o) : hb_iter_with_fallback_t<machine_index_t<Iter>, typename Iter::item_t>(o), it (o.it) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it relevant?
I feel it belongs to a separated issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to mention. Unrelated harfbuzz build fix for gcc 8 and earlier. Will remove before the final push.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API change looks good to me. I understand that this is prerequisite of your logdecoration change. make sense.

I don't understand your change in harfbuzz header.

Thanks for your review, Xin.

@tstuefe tstuefe marked this pull request as ready for review May 6, 2021 04:38
@openjdk openjdk bot added the rfr Pull request is ready for review label May 6, 2021
@mlbridge
Copy link

mlbridge bot commented May 6, 2021

Webrevs

Copy link
Member

@simonis simonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. I have just two minor questions (see inline).

Thanks for adding the gtest!

static bool very_simple_string_matcher(const char* pattern, const char* s) {
const size_t lp = strlen(pattern);
const size_t ls = strlen(s);
if (ls < lp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !=?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to be able to give prefix pattern which do not cover the whole string.

Comment on lines 797 to 800
result = os::iso8601_time(buffer, sizeof(buffer), false);
tty->print_cr("%s", result);
EXPECT_EQ(result, buffer);
EXPECT_TRUE(very_simple_string_matcher(pattern, result));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intentionally repeat this same test for a second time? Does it provide any benefit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, no, this is an oversight. I'll remove the second test, thanks for catching this.

@tstuefe
Copy link
Member Author

tstuefe commented May 6, 2021

Change looks good. I have just two minor questions (see inline).

Thanks for adding the gtest!

Thanks Volker!

Copy link
Member

@simonis simonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.

@openjdk
Copy link

openjdk bot commented May 6, 2021

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps

Reviewed-by: xliu, simonis, ysuenaga

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 20 new commits pushed to the master branch:

  • ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
  • 3a474d9: 8265612: revise the help info for jmap histo command
  • c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
  • 43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
  • 66191ff: 8266193: BasicJMapTest does not include testHistoParallel methods
  • 36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
  • 0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
  • 52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
  • 04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
  • a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/a86ee9b3f370b59caea2ae78169d13498560cd8e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 6, 2021
@tstuefe
Copy link
Member Author

tstuefe commented May 6, 2021

Looks good, thanks.

Thank you Volker!

// Fill in buffer with an ISO-8601 string corresponding to the given javaTimeMillis value
// E.g., YYYY-MM-DDThh:mm:ss.mmm+zzzz.
// Returns buffer, or NULL if it failed.
static char* iso8601_time(jlong milliseconds_since_19700101, char buffer[29],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are supposed to get a compiler warning on this char buffer[29]. ISO C/C++ can't pass in array type as a function argument. As a result, the type buffer automatically casts to char* here.

Further, the function signature is different from its implementation, which use char* buffer instead.
I see that you do enforce length checking in implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch, this is a stray edit I thought I reverted. See below.

ISO C/C++ can't pass in array type as a function argument. As a result, the type buffer automatically casts to char* here.

No, it can, that's totally fine. It gets automatically translated to pointer type, but its a valid expression nonetheless.

It is usually done to indicate that a function expects an output array of a particular length instead of letting the caller guess the right length - and letting him to deal with truncation if he guesses wrong.

I originally planned to define the function as

iso8601_time(jlong milliseconds_since_19700101, char buffer[29]);

to indicate that the only output value length which makes sense is 29, but then I refrained because I wanted to keep the patch simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C/C++ allow you to write code like that, but they won't check the length of argument <=29 for you.
I think you sanity check inside of iso8601_time() is good enough.

@tstuefe
Copy link
Member Author

tstuefe commented May 7, 2021

Thanks @navyxliu and @simonis and @YaSuenag for your reviews.

Last change:

  • fixes the stray edit in the prototype Xin noted
  • adds an explicit constant for the output buffer size of iso timestamps

@YaSuenag could you please officially approve this PR? Unfortunately, Xin is no committer yet, so I need a second reviewer.

Thanks, Thomas

Copy link
Member

@YaSuenag YaSuenag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
I think needed_buffer in os::iso8601_time() is no longer needed, but it's ok if it is left.

@tstuefe
Copy link
Member Author

tstuefe commented May 7, 2021

Looks good!
I think needed_buffer in os::iso8601_time() is no longer needed, but it's ok if it is left.

Thanks Yasumasa! I changed the code a bit and removed needed_buffer.

Copy link
Member

@YaSuenag YaSuenag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing it! It still looks good.

@tstuefe
Copy link
Member Author

tstuefe commented May 7, 2021

/integrate

@openjdk openjdk bot closed this May 7, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2021
@openjdk
Copy link

openjdk bot commented May 7, 2021

@tstuefe Since your change was applied there have been 21 commits pushed to the master branch:

  • 71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
  • ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
  • 3a474d9: 8265612: revise the help info for jmap histo command
  • c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
  • 43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
  • 66191ff: 8266193: BasicJMapTest does not include testHistoParallel methods
  • 36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
  • 0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
  • 52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
  • 04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
  • ... and 11 more: https://git.openjdk.java.net/jdk/compare/a86ee9b3f370b59caea2ae78169d13498560cd8e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 94c6177.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@tstuefe tstuefe deleted the JDK-8266536-os-iso6801-for-arbitrary-timestamps branch June 29, 2021 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants