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.

Sign up for GitHub

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

Jump to bottom

Improve support for resource registration #3315

Merged
merged 1 commit into from
Jun 12, 2021
Merged

Improve support for resource registration #3315

merged 1 commit into from
Jun 12, 2021

Conversation

jovanstevanovic
Copy link
Member

@jovanstevanovic jovanstevanovic commented Mar 29, 2021 •
edited
Loading

Task and high-level overview (#3326)

PROGRESS:

  • First step toward creating a custom filesystem with a TODO list is committed.

@graalvmbot
Copy link
Collaborator

Hello jovanstevanovic, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jovan -(dot)- stevanovic1 -(at)- outlook -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

christianwimmer
christianwimmer reviewed Mar 29, 2021
View reviewed changes
Copy link

@christianwimmer christianwimmer left a comment

Choose a reason for hiding this comment

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

So this PR removes some substitutions that are not necessary because the JDK code eventually calls a method that remains substituted. I'm curious - where is the actual "root" in the JDK that remains substituted?

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/JavaLangSubstitutions.java Outdated
private static URL findResource(String mn, String name) {
return ClassLoader.getSystemClassLoader().getResource(name);
}
@AnnotateOriginal
Copy link

Choose a reason for hiding this comment

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

What is the AnnotateOriginal for? You are not adding any annotations, so this is not going to do anything

@jovanstevanovic jovanstevanovic mentioned this pull request Apr 8, 2021
Closed
@graalvmbot
Copy link
Collaborator

Hello Jovan Stevanovic, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address 44313413+jovanstevanovic -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@jovanstevanovic jovanstevanovic force-pushed the resources branch 3 times, most recently from 2934749 to 4849dff Compare April 21, 2021 12:04
@jovanstevanovic jovanstevanovic force-pushed the resources branch 3 times, most recently from 3e34247 to a92654a Compare April 26, 2021 08:52
@jovanstevanovic jovanstevanovic force-pushed the resources branch 2 times, most recently from 9985fe3 to e2a6942 Compare May 3, 2021 23:15
vjovanov
vjovanov reviewed May 25, 2021
View reviewed changes
...evm/src/com.oracle.svm.core.jdk8/src/com/oracle/svm/core/jdk/resources/ByteArrayChannel.java Outdated
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class ByteArrayChannel implements SeekableByteChannel {
Copy link
Member

Choose a reason for hiding this comment

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

Can we simply ignore the JDK 8 implementation? Would it still work for the set of programs that worked before this PR?

vjovanov
vjovanov reviewed May 25, 2021
View reviewed changes
...m/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/JDKVersionSpecificResourceBuilder.java
import java.net.URL;
import java.net.URLConnection;

public class JDKVersionSpecificResourceBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Then we could skip all of this as well.

This was referenced May 26, 2021
Closed
Closed
@jovanstevanovic jovanstevanovic mentioned this pull request May 27, 2021
Closed
@jovanstevanovic jovanstevanovic force-pushed the resources branch 2 times, most recently from 3dd2ca3 to 84e9b6d Compare May 28, 2021 16:21
@jovanstevanovic jovanstevanovic mentioned this pull request May 29, 2021
Closed
@jovanstevanovic jovanstevanovic force-pushed the resources branch 3 times, most recently from 7ebd3e7 to 7b814c0 Compare June 1, 2021 11:58
@jovanstevanovic jovanstevanovic force-pushed the resources branch 3 times, most recently from 069a8ff to 9931673 Compare June 8, 2021 08:01
gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...tevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/ResourceStorageEntry.java Outdated
return isDirectory;
}

public void setDirectory(boolean directory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This and setData don't seem to be used - I would remove them

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree.

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ResourcesFeature.java Outdated
* </p>
*
* <p>
* ResourceFileSystemProvider exposes methods for creating a new file system, getting existing one
Copy link
Contributor

Choose a reason for hiding this comment

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

I would tweak this comment a bit:

The {@link NativeImageResourceFileSystemProvider} provides most of the functionality of a {@link FileSystem}. It is an in-memory file system that upon creation contains a copy of the resources included in the native-image.
Note that changes to files do not affect actual resources returned by resource manipulation methods like  `Class.getResource`. Upon being closed, all changes are discarded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ResourcesFeature.java Outdated
* </p>
*
* <p>
* Most of operations mentioned above aren't accessed directly i.e. by calling methods from this
Copy link
Contributor

Choose a reason for hiding this comment

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

I would tweak this one as well:

As with other file system providers, these methods provide a low-level interface and are not meant for direct usage - see {@link java.nio.file.Files}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileSystem.java Outdated
new HashSet<>(Arrays.asList("basic", "resource")));

private final Set<InputStream> streams = Collections.synchronizedSet(new HashSet<>());
private final Set<Path> tmpPaths = Collections.synchronizedSet(new HashSet<Path>());
Copy link
Contributor

Choose a reason for hiding this comment

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

<Path> is redundant here -> <>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

...com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileSystem.java Outdated
* <p>
* Some parts of code from this class are copied from jdk.nio.zipfs.ZipFileSystem with small tweaks.
* The main reason why we cannot reuse this class (without any code copying) is that we cannot
* extend jdk.nio.zipfs.ZipPath which is central class in Zip file system.
Copy link
Contributor

Choose a reason for hiding this comment

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

is central class in Zip file system -> is the central class in the Zip file system

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
.../com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileStore.java Outdated
private final long size;

ResourceFileStoreAttributes(NativeImageResourceFileStore fileStore) throws IOException {
Path path = FileSystems.getDefault().getPath(fileStore.name());
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood correctly, a NativeImageResourceFileStore is bound to a path inside of the native-image file system. However, here we seem to be resolving that path in the default file system - I may have missed something, but it seems like this file isn't connected to the native-image file system and the information here, provided that such a path exists, most likely isn't what we're looking to return

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...ratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_java_lang_ClassLoader.java
@@ -229,46 +331,6 @@ public static ClassLoader getSystemClassLoader() {
@Delete
private static native void initSystemClassLoader();

@Substitute
private URL getResource(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I understand these correctly, we've pushed these substitutions to the appropriate class loaders - we now respect the class loader hierarchy when looking up resources

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. That was the idea at some point, but due to a lack of module support, I was forced to return almost all substitutions. Once we get full module support, we will surely push substitutions to appropriate class loaders.

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileSystem.java
} else {
checkParents(dst);
}
Entry target = new Entry(eSrc, Entry.COPY); // Copy eSrc entry.
Copy link
Contributor

Choose a reason for hiding this comment

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

When we do a copy of the entry, we may either create a copy of the original entry's bytes, or, if the previous entry is getting deleted, take the original entry's bytes directly. In both cases, however, copyOnWrite remains false - if we attempt to write data to this entry (using getBytes), it seems like we will accidentally reload the entry - should we also flip the copyOnWrite flag to true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. That could cause a bug in the future.

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileSystem.java Outdated
if (!isTrue(env, "create")) {
throw new FileSystemNotFoundException(resourcePath.toString());
}
this.useTempFile = isTrue(env, "useTempFile");
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood it correctly, this option allows us to create temporary files when modifying a file system entry?
Would such a flag be required? This seems to only be used when attempting to create new output streams, but it may help reduce complexity if we remove it and keep everything in memory

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. No, it's not required, we can get rid of this flag and simplify the operation of creating a new output stream and keep everything in memory.

gradinac
gradinac reviewed Jun 9, 2021
View reviewed changes
...com.oracle.svm.core/src/com/oracle/svm/core/jdk/resources/NativeImageResourceFileSystem.java Outdated
} else {
os = new ByteArrayOutputStream((e.size > 0) ? e.size : 8192);
}
return new EntryOutputStream(e, os);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep everything in memory, would it make sense to also close the output streams when closing the file system?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. I overlooked it. We are keeping all information about opened input streams, so we should do the same thing for output streams.

@jovanstevanovic jovanstevanovic force-pushed the resources branch 6 times, most recently from 6e49565 to 6931388 Compare June 10, 2021 15:47
@graalvmbot graalvmbot merged commit b4c51ca into oracle:master Jun 12, 2021
@jovanstevanovic jovanstevanovic deleted the resources branch June 13, 2021 15:31
@jperedadnr jperedadnr mentioned this pull request Jun 17, 2021
Merged
@olpaw olpaw mentioned this pull request Jun 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vjovanov vjovanov vjovanov left review comments

@christianwimmer christianwimmer christianwimmer left review comments

@gradinac gradinac gradinac left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jovanstevanovic @graalvmbot @vjovanov @christianwimmer @gradinac