Release app image metadata when startup is completed
The app image startup cache is only required for startup. Release
it after startup is completed.
Note that the cache handles being zeroed out at any time.
Bug: 123377072
Test: test-art-host
Change-Id: Iaa1c800bd0de7290b44d814a21bda20298a10d5f
diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc
index c5c5e8a..1a145ec 100644
--- a/runtime/gc/space/image_space.cc
+++ b/runtime/gc/space/image_space.cc
@@ -2417,6 +2417,42 @@
}
}
+void ImageSpace::DisablePreResolvedStrings() {
+ // Clear dex cache pointers.
+ ObjPtr<mirror::ObjectArray<mirror::DexCache>> dex_caches =
+ GetImageHeader().GetImageRoot(ImageHeader::kDexCaches)->AsObjectArray<mirror::DexCache>();
+ for (size_t len = dex_caches->GetLength(), i = 0; i < len; ++i) {
+ ObjPtr<mirror::DexCache> dex_cache = dex_caches->Get(i);
+ dex_cache->ClearPreResolvedStrings();
+ }
+}
+
+void ImageSpace::ReleaseMetadata() {
+ const ImageSection& metadata = GetImageHeader().GetMetadataSection();
+ VLOG(image) << "Releasing " << metadata.Size() << " image metadata bytes";
+ // In the case where new app images may have been added around the checkpoint, ensure that we
+ // don't madvise the cache for these.
+ ObjPtr<mirror::ObjectArray<mirror::DexCache>> dex_caches =
+ GetImageHeader().GetImageRoot(ImageHeader::kDexCaches)->AsObjectArray<mirror::DexCache>();
+ bool have_startup_cache = false;
+ for (size_t len = dex_caches->GetLength(), i = 0; i < len; ++i) {
+ ObjPtr<mirror::DexCache> dex_cache = dex_caches->Get(i);
+ if (dex_cache->NumPreResolvedStrings() != 0u) {
+ have_startup_cache = true;
+ }
+ }
+ // Only safe to do for images that have their preresolved strings caches disabled. This is because
+ // uncompressed images madvise to the original unrelocated image contents.
+ if (!have_startup_cache) {
+ // Avoid using ZeroAndReleasePages since the zero fill might not be word atomic.
+ uint8_t* const page_begin = AlignUp(Begin() + metadata.Offset(), kPageSize);
+ uint8_t* const page_end = AlignDown(Begin() + metadata.End(), kPageSize);
+ if (page_begin < page_end) {
+ CHECK_NE(madvise(page_begin, page_end - page_begin, MADV_DONTNEED), -1) << "madvise failed";
+ }
+ }
+}
+
} // namespace space
} // namespace gc
} // namespace art
diff --git a/runtime/gc/space/image_space.h b/runtime/gc/space/image_space.h
index c7d5140..a3a9557 100644
--- a/runtime/gc/space/image_space.h
+++ b/runtime/gc/space/image_space.h
@@ -172,6 +172,9 @@
// De-initialize the image-space by undoing the effects in Init().
virtual ~ImageSpace();
+ void DisablePreResolvedStrings() REQUIRES_SHARED(Locks::mutator_lock_);
+ void ReleaseMetadata() REQUIRES_SHARED(Locks::mutator_lock_);
+
protected:
// Tries to initialize an ImageSpace from the given image path, returning null on error.
//
diff --git a/runtime/mirror/dex_cache-inl.h b/runtime/mirror/dex_cache-inl.h
index f0ad931..c5a32ef 100644
--- a/runtime/mirror/dex_cache-inl.h
+++ b/runtime/mirror/dex_cache-inl.h
@@ -86,11 +86,16 @@
inline String* DexCache::GetResolvedString(dex::StringIndex string_idx) {
const uint32_t num_preresolved_strings = NumPreResolvedStrings();
if (num_preresolved_strings != 0u) {
- DCHECK_LT(string_idx.index_, num_preresolved_strings);
- DCHECK_EQ(num_preresolved_strings, GetDexFile()->NumStringIds());
- mirror::String* string = GetPreResolvedStrings()[string_idx.index_].Read();
- if (LIKELY(string != nullptr)) {
- return string;
+ GcRoot<mirror::String>* preresolved_strings = GetPreResolvedStrings();
+ // num_preresolved_strings can become 0 and preresolved_strings can become null in any order
+ // when ClearPreResolvedStrings is called.
+ if (preresolved_strings != nullptr) {
+ DCHECK_LT(string_idx.index_, num_preresolved_strings);
+ DCHECK_EQ(num_preresolved_strings, GetDexFile()->NumStringIds());
+ mirror::String* string = preresolved_strings[string_idx.index_].Read();
+ if (LIKELY(string != nullptr)) {
+ return string;
+ }
}
}
return GetStrings()[StringSlotIndex(string_idx)].load(
diff --git a/runtime/mirror/dex_cache.h b/runtime/mirror/dex_cache.h
index b5619f8..3e433c9 100644
--- a/runtime/mirror/dex_cache.h
+++ b/runtime/mirror/dex_cache.h
@@ -283,8 +283,7 @@
ObjPtr<mirror::String> resolved)
ALWAYS_INLINE REQUIRES_SHARED(Locks::mutator_lock_);
- // Clear the preresolved string cache to prevent further usage. Not thread safe, so should only
- // be called when the string cache is guaranteed to not be accessed.
+ // Clear the preresolved string cache to prevent further usage.
void ClearPreResolvedStrings()
ALWAYS_INLINE REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 4565939..3e8b7c8 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -2788,6 +2788,33 @@
}
VLOG(startup) << "Startup completed notified";
+ {
+ ScopedTrace trace("Releasing app image spaces metadata");
+ ScopedObjectAccess soa(Thread::Current());
+ for (gc::space::ContinuousSpace* space : GetHeap()->GetContinuousSpaces()) {
+ if (space->IsImageSpace()) {
+ gc::space::ImageSpace* image_space = space->AsImageSpace();
+ if (image_space->GetImageHeader().IsAppImage()) {
+ image_space->DisablePreResolvedStrings();
+ }
+ }
+ }
+ // Request empty checkpoint to make sure no threads are accessing the section when we madvise
+ // it.
+ {
+ ScopedThreadStateChange tsc(Thread::Current(), kSuspended);
+ GetThreadList()->RunEmptyCheckpoint();
+ }
+ for (gc::space::ContinuousSpace* space : GetHeap()->GetContinuousSpaces()) {
+ if (space->IsImageSpace()) {
+ gc::space::ImageSpace* image_space = space->AsImageSpace();
+ if (image_space->GetImageHeader().IsAppImage()) {
+ image_space->ReleaseMetadata();
+ }
+ }
+ }
+ }
+
// Notify the profiler saver that startup is now completed.
ProfileSaver::NotifyStartupCompleted();
diff --git a/test/1003-metadata-section-strings/build b/test/1003-metadata-section-strings/build
new file mode 100755
index 0000000..cd2cacd
--- /dev/null
+++ b/test/1003-metadata-section-strings/build
@@ -0,0 +1,41 @@
+#!/bin/bash
+#
+# Copyright 2019 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# make us exit on a failure
+set -e
+
+count=5000
+
+# Generate the other class.
+other_file="src-art/Other.java"
+cat >${other_file} <<EOF
+class Other {
+static String getString(int i) {
+switch(i) {
+EOF
+
+for i in $(seq 1 "$count"); do
+ echo " case ${i}: return \"string$i\";" >> "${other_file}"
+done
+
+cat >>${other_file} <<EOF
+}
+return "not found";
+}
+}
+EOF
+
+./default-build "$@"
diff --git a/test/1003-metadata-section-strings/expected.txt b/test/1003-metadata-section-strings/expected.txt
new file mode 100644
index 0000000..8925e1f
--- /dev/null
+++ b/test/1003-metadata-section-strings/expected.txt
@@ -0,0 +1,9 @@
+true
+true
+true
+true
+After startup completed
+true
+true
+true
+true
diff --git a/test/1003-metadata-section-strings/info.txt b/test/1003-metadata-section-strings/info.txt
new file mode 100644
index 0000000..38ed48a
--- /dev/null
+++ b/test/1003-metadata-section-strings/info.txt
@@ -0,0 +1 @@
+Test that releasing the metadata cache doesn't break reference equality of string literals.
\ No newline at end of file
diff --git a/test/1003-metadata-section-strings/profile b/test/1003-metadata-section-strings/profile
new file mode 100644
index 0000000..0e008b4
--- /dev/null
+++ b/test/1003-metadata-section-strings/profile
@@ -0,0 +1 @@
+SLOther;->getString(I)V
diff --git a/test/1003-metadata-section-strings/run b/test/1003-metadata-section-strings/run
new file mode 100644
index 0000000..9762aba
--- /dev/null
+++ b/test/1003-metadata-section-strings/run
@@ -0,0 +1,17 @@
+#!/bin/bash
+#
+# Copyright (C) 2019 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+exec ${RUN} $@ --profile -Xcompiler-option --compiler-filter=speed-profile -Xcompiler-option --resolve-startup-const-strings=true
diff --git a/test/1003-metadata-section-strings/src-art/Main.java b/test/1003-metadata-section-strings/src-art/Main.java
new file mode 100644
index 0000000..99e1fbd
--- /dev/null
+++ b/test/1003-metadata-section-strings/src-art/Main.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import dalvik.system.VMRuntime;
+
+class Main {
+ public static void main(String[] args) {
+ runTest();
+ try {
+ VMRuntime.getRuntime().notifyStartupCompleted();
+ } catch (Throwable e) {
+ System.out.println("Exception finishing startup " + e);
+ }
+ System.out.println("After startup completed");
+ runTest();
+ }
+
+ private static void runTest() {
+ // Test that reference equality is sane regarding the cache.
+ System.out.println(Other.getString(1) == "string1");
+ System.out.println(Other.getString(2000) == "string2000");
+ System.out.println(Other.getString(3000) == "string3000");
+ System.out.println(Other.getString(5000) == "string5000");
+ }
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 6a41daf..0206b15 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1063,6 +1063,7 @@
"989-method-trace-throw",
"993-breakpoints",
"1002-notify-startup",
+ "1003-metadata-section-strings",
"1900-track-alloc",
"1906-suspend-list-me-first",
"1914-get-local-instance",