8231640: (prop) Canonical property storage #5372
Conversation
|
I haven't yet created a CSR for this and will do that next week once the initial reviews and changes are sorted out. |
|
|
Webrevs
|
| synchronized (this) { | ||
| for (Map.Entry<Object, Object> e : entrySet()) { | ||
| for (Map.Entry<Object, Object> e : new TreeMap<>(map).entrySet()) { |
There was a problem hiding this comment.
Is this sorting intentionally added? It's not clear from issue description or PR description that order of properties should be changed too.
Anyway I think copying entrySet() to array and then sorting should be faster, than creating a TreeMap
There was a problem hiding this comment.
In case of reproducibility it should be at least ordered, i.e. keep original input order.
|
Mailing list message from Alan Bateman on core-libs-dev: On 04/09/2021 16:50, Jaikiran Pai wrote:
In the discussion on this option then I think we were talking about The SM case probably needs discussion as I was expecting to see PrivilegedAction<String> pa = () -> System.getenv("SOURCE_DATE_EPOCH"); as a caller of the store method (in a library for example) is unlikely
The store methods are already specified to throw CCE if the properties -Alan |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Hello Alan, On 05/09/21 1:46 pm, Alan Bateman wrote:
I will move the updated javadoc to an @implNote then. I guess, the
I had initially started off with using a doPrivileged code in this In such a doPrivelged case, the only way the administrator can prevent So the doPriveleged case, IMO, will not allow selective granting of -Jaikiran |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Hello Andrey, On 05/09/21 12:02 am, Andrey Turbanov wrote:
Yes, the ordering of the properties is intentional as noted in the As for the usage of TreeMap, I had looked around the JDK code to see if -Jaikiran |
|
Off topic - the |
|
|
Mailing list message from Jaikiran Pai on core-libs-dev: On 05/09/21 6:01 pm, Jaikiran Pai wrote:
I've now updated the PR to move the new text of this javadoc into a -Jaikiran |
You can convert entrySet() to array. Not a keySet. In this case there is no need to lookup for values. |
|
Mailing list message from Roger Riggs on core-libs-dev: Hi, The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the The complexity in the specification and implementation seem unnecessary Though java.util.Date is used in the current implementation, its use is Thanks, Roger On 9/5/21 8:31 AM, Jaikiran Pai wrote:
|
|
Mailing list message from Jaikiran Pai on core-libs-dev: On 07/09/21 8:35 pm, Roger Riggs wrote:
Given the inputs so far, the doPriveleged approach appears to be
Noted. I'll take a look at these and update the PR as necessary. Thank you all for the inputs so far. -Jaikiran |
|
Mailing list message from Alan Bateman on core-libs-dev: On 07/09/2021 16:05, Roger Riggs wrote:
I agree.? Given the complexity then it makes your suggestion/option to -Alan |
|
Mailing list message from Stuart Marks on core-libs-dev: On 9/7/21 8:27 AM, Jaikiran Pai wrote:
Unless there's an overriding reason, it might be nice to have the output format https://salsa.debian.org/openjdk-team/openjdk/-/blob/master/debian/patches/reproducible-properties-timestamp.diff (See JDK-8272157 for the journey that led us here, in particular, lack of s'marks |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Hello Robert, On 07/09/21 11:24 pm, Robert Scholte wrote:
As discussed in the mailing list, it is agreed upon that these property -Jaikiran |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Hello Stuart, On 08/09/21 6:49 am, Stuart Marks wrote:
So the current patch implementation uses the format "d MMM yyyy HH:mm:ss Roger's suggestion is to use DateTimeFormatter#RFC_1123_DATE_TIME date IMO, either of these formats are "well known", since they are/were used The one in the debian patch is "yyyy-MM-dd HH:mm:ss z" which although is I was leaning towards Roger's suggestion to use the RFC_1123_DATE_TIME -Jaikiran |
- Use doPriveleged instead of explicit permission checks, to reduce complexity - Use DateTimeFormatter and ZonedDateTime instead of Date.toString() - Use DateTimeFormatter.RFC_1123_DATE_TIME for formatting SOURCE_DATE_EPOCH dates - Use Arrays.sort instead of a TreeMap for ordering property keys
|
Mailing list message from Jaikiran Pai on core-libs-dev: On 07/09/21 9:02 pm, Alan Bateman wrote:
I've now updated the PR to take into account the inputs that were ?- remove the complexity around SecurityManager usage and now just uses ?- use Arrays.sort(...) instead of a TreeMap to write out the sorted ?- use DateTimeFormatter.RFC_1123_DATE_TIME while formatting and ?- use ZonedDateTime along with a DateTimeFormatter which matches the The new tests that have been introduced in this PR have been adjusted to -Jaikiran |
| bw.newLine(); | ||
| synchronized (this) { | ||
| for (Map.Entry<Object, Object> e : entrySet()) { | ||
| var entries = map.entrySet().toArray(new Map.Entry<?, ?>[0]); | ||
| Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() { |
There was a problem hiding this comment.
This part here, intentionally doesn't use a lambda, since from what I remember seeing in some mail discussion, it was suggested that using lambda in core parts which get used very early during JVM boostrap, should be avoided. If that's not a concern here, do let me know and I can change it to a lambda.
There was a problem hiding this comment.
This is a fair concern, but writing out a properties file is a pretty high-level operation that doesn't seem likely to be used during bootstrap. Also, I observe that the doPrivileged block above uses a lambda. So I think we're probably ok to use a lambda here. But if you get an inexplicable error at build time or at startup time, this would be the reason why.
Assuming we're ok with lambdas, it might be easier to use collections instead of arrays in order to preserve generic types. Unfortunately the map is Map<Object, Object> so we have to do some fancy casting to get the right type. But then we can use Map.Entry.comparingByKey() as the comparator. (Note that this uses lambda internally.)
Something like this might work:
@SuppressWarnings("unchecked")
var entries = new ArrayList<>(((Map<String, String>)(Map)map).entrySet());
entries.sort(Map.Entry.comparingByKey());
|
Mailing list message from Jaikiran Pai on core-libs-dev: Hello Andrey, On 07/09/21 7:50 pm, Andrey Turbanov wrote:
I experimented this in a JMH test and the results matched your package org.myapp; import org.openjdk.jmh.annotations.Benchmark; public class MyBenchmark { ??? @State(Scope.Thread) ??????? static { ??? @Benchmark ??? @Benchmark ??? @Benchmark ??? @Benchmark ??? @Benchmark ??? @Benchmark } Results: Benchmark??????????????????????????????????? Mode? Cnt??? Score Error? Units -Jaikiran |
|
Am I the only one thinking there should also be a way for developers to explicitly disable timestamps from the API? I think the current iteration looks okay (but the core-libs guys of course has the say in this), but I still think we need a new method (or overload) to allow for a timestamp-free to be generated, always, independent of environment variables. |
|
Mailing list message from Stuart Marks on core-libs-dev:
My point in bringing this is up is to consider interoperability. I don't have a 1) Baseline OpenJDK 17 behavior: dow mon dd hh:mm:ss zzz yyyy This is the behavior provided by "new Date().toString()" and has likely not changed 2) Debian's OpenJDK with SOURCE_DATE_EPOCH set: yyyy-MM-dd HH:mm:ss z The question is, what format should the JDK-8231640 use? I had said earlier that it might be a good idea to match the Debian format. But So the more specific question is, should we try to continue with the original JDK s'marks |
- Implement Roger's suggestion to explicitly state that the system property should be set on command line - Change @implNote to @implSpec based on inputs being provided on CSR issue - Use Roger's and Daniel's suggestions to reword the @implSpec text to explain how overridding of entrySet() can impact the order of the written out properties - Improve the internal code comment (meant for maintainers) to be more clear on what kind of check we are doing before deciding the order of properties to write
|
Mailing list message from Jaikiran Pai on core-libs-dev: On 16/09/21 4:05 am, Roger Riggs wrote:
To summarize the options that we have discussed for this entrySet() part: - Do nothing specific for subclasses that override entrySet() method. - Check the Properties object instance type to see if it has been - Detect that the entrySet() method is overridden by the subclass of -Jaikiran |
…storeDate system property to java.properties.date - Tests updated accordingly
…e in the list of system properties listed in System::getProperties()
|
What would be the next step to move the CSR forward? Should I be changing it's status or do something else? |
|
Mailing list message from Joseph D. Darcy on core-libs-dev: Hello Jaikiran, The CSR is in Draft state. As discussed in the CSR wiki HTH, -Joe On 9/20/2021 8:46 PM, Jaikiran Pai wrote: |
|
Mailing list message from Jaikiran Pai on core-libs-dev: Thank you Joe. That link helped. I have now moved the CSR to the next state. -Jaikiran On 21/09/21 9:28 am, Joseph D. Darcy wrote:
|
| // returned by the Properties.entrySet() implementation. If yes, then we sort those entries | ||
| // in the natural order of their key. Else, we consider that the subclassed implementation may potentially | ||
| // have returned a differently ordered entries and so we just use the iteration order of the returned instance. | ||
| if (entries instanceof Collections.SynchronizedSet<?> ss |
There was a problem hiding this comment.
Would you mind re-formatting the comment to keep the line length a bit more consistent with the rest of the code, is's just a bit annoying to have it wrapping.
There was a problem hiding this comment.
Done. I've updated the PR to reduce the line length of that code comment.
| @@ -761,6 +761,9 @@ public static native void arraycopy(Object src, int srcPos, | |||
| * <tr><th scope="row">{@systemProperty native.encoding}</th> | |||
| * <td>Character encoding name derived from the host environment and/or | |||
| * the user's settings. Setting this system property has no effect.</td></tr> | |||
| * <tr><th scope="row">{@systemProperty java.properties.date}</th> | |||
| * <td>Text for the comment that must replace the default date comment | |||
| * written out by {@code Properties.store()} methods <em>(optional)</em> </td></tr> | |||
There was a problem hiding this comment.
To date, the table in getProperties has listed the supported system properties that the runtime makes available to applications. It hasn't historically listed the many other standard properties that can be set on the command line. So I'm sure about adding this one to the table as it opens the door to expanding the table to all the other system properties that are documented elsewhere in the API docs. Note that javadoc creates a table of system properties from usages of @systemProperty so there is already a more complete table in the javadoc build.
There was a problem hiding this comment.
After this change to include this property in System::getProperties() javadoc was added, there have been inputs (here and on the CSR) which suggest that we probably shouldn't include it here. I've now updated this PR to revert this specific change.
…ties.date in the list of system properties listed in System::getProperties()" Additional inputs since this specific commit was introduced have leaned towards not listing this property in System::getProperties() This reverts commit 458c1fd.
|
@jaikiran This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: 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 173 new commits pushed to the
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.
|
|
The CSR for this has been approved and there are no pending changes in this PR. I am thinking of integrating this PR in around 24 hours from now if this looks good. |
|
I agree with Roger. It's hard to understand the amount of work you have put into this when you only look at the small, resulting patch. Thank you for pulling this through! |
|
It was a pleasure working on this one due to the timely and valuable reviews, inputs and suggestions. Thanks to everyone for helping in getting this done. |
|
/integrate |
|
Going to push as commit af50772.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
The commit in this PR implements the proposal for enhancement that was discussed in the core-libs-dev mailing list recently[1], for https://bugs.openjdk.java.net/browse/JDK-8231640
At a high level - the
store()APIs inPropertieshave been modified to now look for theSOURCE_DATE_EPOCHenvironment variable[2]. If that env variable is set, then instead of writing out the current date time as a date comment, thestore()APIs instead will use the value set for this env variable to parse it to aDateand write out the string form of such a date. The implementation here uses thed MMM yyyy HH:mm:ss 'GMT'date format andLocale.ROOTto format and write out such a date. This should provide reproducibility whenever theSOURCE_DATE_EPOCHis set. Furthermore, intentionally, no changes in the date format of the "current date" have been done.These modified
store()APIs work in the presence of theSecurityManagertoo. The caller is expected to have a read permission on theSOURCE_DATE_EPOCHenvironment variable. If the caller doesn't have that permission, then the implementation of thesestore()APIs will write out the "current date" and will ignore any value that has been set for theSOURCE_DATE_EPOCHenv variable. This should allow for backward compatibility of existing applications, where, when they run under aSecurityManagerand perhaps with an existing restrictive policy file, the presence ofSOURCE_DATE_EPOCHshouldn't impact their calls to thestore()APIs.The modified
store()APIs will also ignore any value forSOURCE_DATE_EPOCHthat cannot be parsed to anlongvalue. In such cases, thestore()APIs will write out the "current date" and ignore the value set for this environment variable. No exceptions will be thrown for such invalid values. This is an additional backward compatibility precaution to prevent any rogue value forSOURCE_DATE_EPOCHfrom breaking applications.An additional change in the implementation of these
store()APIs and unrelated to the date comment, is that these APIs will now write out the property keys in a deterministic order. The keys will be written out in the natural ordering as specified byjava.lang.String#compareTo()API.The combination of the ordering of the property keys when written out and the usage of
SOURCE_DATE_EPOCHenvironment value to determine the date comment should together allow for reproducibility of the output generated by thesestore()APIs.New jtreg test classes have been introduced to verify these changes. The primary focus of
PropertiesStoreTestis the ordering aspects of the property keys that are written out. On the other handStoreReproducibilityTestfocuses on the reproducibility of the output generated by these APIs. TheStoreReproducibilityTestruns these tests both in the presence and absence ofSecurityManager. Plus, in the presence of SecurityManager, it tests both the scenarios where the caller is granted the requisite permission and in other case not granted that permission.These new tests and existing tests under
test/jdk/java/util/Properties/pass with these changes.[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html
[2] https://reproducible-builds.org/specs/source-date-epoch/
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5372/head:pull/5372$ git checkout pull/5372Update a local copy of the PR:
$ git checkout pull/5372$ git pull https://git.openjdk.java.net/jdk pull/5372/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5372View PR using the GUI difftool:
$ git pr show -t 5372Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5372.diff