diff options
17 files changed, 143 insertions, 26 deletions
diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index 138863ac38..d0215255e8 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -1194,15 +1194,18 @@ bool CompilerDriver::CanAccessTypeWithoutChecks(uint32_t referrer_idx, const Dex if (equals_referrers_class != nullptr) { *equals_referrers_class = (method_id.class_idx_ == type_idx); } - mirror::Class* referrer_class = dex_cache->GetResolvedType(method_id.class_idx_); - if (referrer_class == nullptr) { - stats_->TypeNeedsAccessCheck(); - return false; // Incomplete referrer knowledge needs access check. - } - // Perform access check, will return true if access is ok or false if we're going to have to - // check this at runtime (for example for class loaders). - bool result = referrer_class->CanAccess(resolved_class); - if (result) { + bool is_accessible = resolved_class->IsPublic(); // Public classes are always accessible. + if (!is_accessible) { + mirror::Class* referrer_class = dex_cache->GetResolvedType(method_id.class_idx_); + if (referrer_class == nullptr) { + stats_->TypeNeedsAccessCheck(); + return false; // Incomplete referrer knowledge needs access check. + } + // Perform access check, will return true if access is ok or false if we're going to have to + // check this at runtime (for example for class loaders). + is_accessible = referrer_class->CanAccess(resolved_class); + } + if (is_accessible) { stats_->TypeDoesntNeedAccessCheck(); if (type_known_final != nullptr) { *type_known_final = resolved_class->IsFinal() && !resolved_class->IsArrayClass(); @@ -1213,7 +1216,7 @@ bool CompilerDriver::CanAccessTypeWithoutChecks(uint32_t referrer_idx, const Dex } else { stats_->TypeNeedsAccessCheck(); } - return result; + return is_accessible; } bool CompilerDriver::CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_idx, @@ -1233,14 +1236,18 @@ bool CompilerDriver::CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_id } *finalizable = resolved_class->IsFinalizable(); const DexFile::MethodId& method_id = dex_file.GetMethodId(referrer_idx); - mirror::Class* referrer_class = dex_cache->GetResolvedType(method_id.class_idx_); - if (referrer_class == nullptr) { - stats_->TypeNeedsAccessCheck(); - return false; // Incomplete referrer knowledge needs access check. - } - // Perform access and instantiable checks, will return true if access is ok or false if we're - // going to have to check this at runtime (for example for class loaders). - bool result = referrer_class->CanAccess(resolved_class) && resolved_class->IsInstantiable(); + bool is_accessible = resolved_class->IsPublic(); // Public classes are always accessible. + if (!is_accessible) { + mirror::Class* referrer_class = dex_cache->GetResolvedType(method_id.class_idx_); + if (referrer_class == nullptr) { + stats_->TypeNeedsAccessCheck(); + return false; // Incomplete referrer knowledge needs access check. + } + // Perform access and instantiable checks, will return true if access is ok or false if we're + // going to have to check this at runtime (for example for class loaders). + is_accessible = referrer_class->CanAccess(resolved_class); + } + bool result = is_accessible && resolved_class->IsInstantiable(); if (result) { stats_->TypeDoesntNeedAccessCheck(); } else { diff --git a/compiler/optimizing/ssa_builder.cc b/compiler/optimizing/ssa_builder.cc index 7494e336b1..c0011cde8d 100644 --- a/compiler/optimizing/ssa_builder.cc +++ b/compiler/optimizing/ssa_builder.cc @@ -422,6 +422,34 @@ bool SsaBuilder::FixAmbiguousArrayOps() { return true; } +void SsaBuilder::RemoveRedundantUninitializedStrings() { + if (GetGraph()->IsDebuggable()) { + // Do not perform the optimization for consistency with the interpreter + // which always allocates an object for new-instance of String. + return; + } + + for (HNewInstance* new_instance : uninitialized_strings_) { + DCHECK(new_instance->IsStringAlloc()); + + // Replace NewInstance of String with NullConstant if not used prior to + // calling StringFactory. In case of deoptimization, the interpreter is + // expected to skip null check on the `this` argument of the StringFactory call. + if (!new_instance->HasNonEnvironmentUses()) { + new_instance->ReplaceWith(GetGraph()->GetNullConstant()); + new_instance->GetBlock()->RemoveInstruction(new_instance); + + // Remove LoadClass if not needed any more. + HLoadClass* load_class = new_instance->InputAt(0)->AsLoadClass(); + DCHECK(load_class != nullptr); + DCHECK(!load_class->NeedsAccessCheck()) << "String class is always accessible"; + if (!load_class->HasUses()) { + load_class->GetBlock()->RemoveInstruction(load_class); + } + } + } +} + GraphAnalysisResult SsaBuilder::BuildSsa() { // 1) Visit in reverse post order. We need to have all predecessors of a block // visited (with the exception of loops) in order to create the right environment @@ -487,7 +515,15 @@ GraphAnalysisResult SsaBuilder::BuildSsa() { // input types. dead_phi_elimimation.EliminateDeadPhis(); - // 11) Clear locals. + // 11) Step 1) replaced uses of NewInstances of String with the results of + // their corresponding StringFactory calls. Unless the String objects are used + // before they are initialized, they can be replaced with NullConstant. + // Note that this optimization is valid only if unsimplified code does not use + // the uninitialized value because we assume execution can be deoptimized at + // any safepoint. We must therefore perform it before any other optimizations. + RemoveRedundantUninitializedStrings(); + + // 12) Clear locals. for (HInstructionIterator it(GetGraph()->GetEntryBlock()->GetInstructions()); !it.Done(); it.Advance()) { @@ -894,6 +930,10 @@ void SsaBuilder::VisitInvokeStaticOrDirect(HInvokeStaticOrDirect* invoke) { HNewInstance* new_instance = invoke->GetThisArgumentOfStringInit(); invoke->RemoveThisArgumentOfStringInit(); + // Replacing the NewInstance might render it redundant. Keep a list of these + // to be visited once it is clear whether it is has remaining uses. + uninitialized_strings_.push_back(new_instance); + // Walk over all vregs and replace any occurrence of `new_instance` with `invoke`. for (size_t vreg = 0, e = current_locals_->size(); vreg < e; ++vreg) { if ((*current_locals_)[vreg] == new_instance) { diff --git a/compiler/optimizing/ssa_builder.h b/compiler/optimizing/ssa_builder.h index 28eef6a40c..ccef8ea380 100644 --- a/compiler/optimizing/ssa_builder.h +++ b/compiler/optimizing/ssa_builder.h @@ -57,6 +57,7 @@ class SsaBuilder : public HGraphVisitor { loop_headers_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)), ambiguous_agets_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)), ambiguous_asets_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)), + uninitialized_strings_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)), locals_for_(graph->GetBlocks().size(), ArenaVector<HInstruction*>(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)), graph->GetArena()->Adapter(kArenaAllocSsaBuilder)) { @@ -105,6 +106,8 @@ class SsaBuilder : public HGraphVisitor { HPhi* GetFloatDoubleOrReferenceEquivalentOfPhi(HPhi* phi, Primitive::Type type); HArrayGet* GetFloatOrDoubleEquivalentOfArrayGet(HArrayGet* aget); + void RemoveRedundantUninitializedStrings(); + StackHandleScopeCollection* const handles_; // True if types of ambiguous ArrayGets have been resolved. @@ -119,6 +122,7 @@ class SsaBuilder : public HGraphVisitor { ArenaVector<HArrayGet*> ambiguous_agets_; ArenaVector<HArraySet*> ambiguous_asets_; + ArenaVector<HNewInstance*> uninitialized_strings_; // HEnvironment for each block. ArenaVector<ArenaVector<HInstruction*>> locals_for_; diff --git a/runtime/entrypoints/entrypoint_utils-inl.h b/runtime/entrypoints/entrypoint_utils-inl.h index 9a9f42b976..68f2dcbf31 100644 --- a/runtime/entrypoints/entrypoint_utils-inl.h +++ b/runtime/entrypoints/entrypoint_utils-inl.h @@ -410,10 +410,19 @@ inline ArtMethod* FindMethodFromCode(uint32_t method_idx, mirror::Object** this_ DCHECK(self->IsExceptionPending()); // Throw exception and unwind. return nullptr; // Failure. } else if (UNLIKELY(*this_object == nullptr && type != kStatic)) { - // Maintain interpreter-like semantics where NullPointerException is thrown - // after potential NoSuchMethodError from class linker. - ThrowNullPointerExceptionForMethodAccess(method_idx, type); - return nullptr; // Failure. + if (UNLIKELY(resolved_method->GetDeclaringClass()->IsStringClass() && + resolved_method->IsConstructor())) { + // Hack for String init: + // + // We assume that the input of String.<init> in verified code is always + // an unitialized reference. If it is a null constant, it must have been + // optimized out by the compiler. Do not throw NullPointerException. + } else { + // Maintain interpreter-like semantics where NullPointerException is thrown + // after potential NoSuchMethodError from class linker. + ThrowNullPointerExceptionForMethodAccess(method_idx, type); + return nullptr; // Failure. + } } else if (access_check) { mirror::Class* methods_class = resolved_method->GetDeclaringClass(); bool can_access_resolved_method = diff --git a/runtime/interpreter/interpreter_common.cc b/runtime/interpreter/interpreter_common.cc index 18fb0d8518..ecd4de9bd0 100644 --- a/runtime/interpreter/interpreter_common.cc +++ b/runtime/interpreter/interpreter_common.cc @@ -592,6 +592,10 @@ static inline bool DoCallCommon(ArtMethod* called_method, // // (at this point the ArtMethod has already been replaced, // so we just need to fix-up the arguments) + // + // Note that FindMethodFromCode in entrypoint_utils-inl.h was also special-cased + // to handle the compiler optimization of replacing `this` with null without + // throwing NullPointerException. uint32_t string_init_vreg_this = is_range ? vregC : arg[0]; if (UNLIKELY(string_init)) { DCHECK_GT(num_regs, 0u); // As the method is an instance method, there should be at least 1. diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc index 5c9dab2f38..8f4d24f385 100644 --- a/runtime/jit/jit.cc +++ b/runtime/jit/jit.cc @@ -56,7 +56,8 @@ void Jit::DumpInfo(std::ostream& os) { os << "JIT code cache size=" << PrettySize(code_cache_->CodeCacheSize()) << "\n" << "JIT data cache size=" << PrettySize(code_cache_->DataCacheSize()) << "\n" << "JIT current capacity=" << PrettySize(code_cache_->GetCurrentCapacity()) << "\n" - << "JIT number of compiled code=" << code_cache_->NumberOfCompiledCode() << "\n"; + << "JIT number of compiled code=" << code_cache_->NumberOfCompiledCode() << "\n" + << "JIT total number of compilations=" << code_cache_->NumberOfCompilations() << "\n"; cumulative_timings_.Dump(os); } diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc index 2d575bdb31..64b2c899aa 100644 --- a/runtime/jit/jit_code_cache.cc +++ b/runtime/jit/jit_code_cache.cc @@ -125,7 +125,8 @@ JitCodeCache::JitCodeCache(MemMap* code_map, data_end_(initial_data_capacity), has_done_one_collection_(false), last_update_time_ns_(0), - garbage_collect_code_(garbage_collect_code) { + garbage_collect_code_(garbage_collect_code), + number_of_compilations_(0) { DCHECK_GE(max_capacity, initial_code_capacity + initial_data_capacity); code_mspace_ = create_mspace_with_base(code_map_->Begin(), code_end_, false /*locked*/); @@ -322,6 +323,7 @@ uint8_t* JitCodeCache::CommitCodeInternal(Thread* self, __builtin___clear_cache(reinterpret_cast<char*>(code_ptr), reinterpret_cast<char*>(code_ptr + code_size)); + number_of_compilations_++; } // We need to update the entry point in the runnable state for the instrumentation. { @@ -347,6 +349,11 @@ uint8_t* JitCodeCache::CommitCodeInternal(Thread* self, return reinterpret_cast<uint8_t*>(method_header); } +size_t JitCodeCache::NumberOfCompilations() { + MutexLock mu(Thread::Current(), lock_); + return number_of_compilations_; +} + size_t JitCodeCache::CodeCacheSize() { MutexLock mu(Thread::Current(), lock_); return CodeCacheSizeLocked(); diff --git a/runtime/jit/jit_code_cache.h b/runtime/jit/jit_code_cache.h index a152bcd2d4..67fa928f61 100644 --- a/runtime/jit/jit_code_cache.h +++ b/runtime/jit/jit_code_cache.h @@ -68,6 +68,9 @@ class JitCodeCache { // of methods that got JIT compiled, as we might have collected some. size_t NumberOfCompiledCode() REQUIRES(!lock_); + // Number of compilations done throughout the lifetime of the JIT. + size_t NumberOfCompilations() REQUIRES(!lock_); + bool NotifyCompilationOf(ArtMethod* method, Thread* self) SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(!lock_); @@ -261,6 +264,9 @@ class JitCodeCache { // Whether we can do garbage collection. const bool garbage_collect_code_; + // Number of compilations done throughout the lifetime of the JIT. + size_t number_of_compilations_ GUARDED_BY(lock_); + DISALLOW_IMPLICIT_CONSTRUCTORS(JitCodeCache); }; diff --git a/test/127-secondarydex/build b/test/127-checker-secondarydex/build index 0d9f4d6291..0d9f4d6291 100755 --- a/test/127-secondarydex/build +++ b/test/127-checker-secondarydex/build diff --git a/test/127-secondarydex/expected.txt b/test/127-checker-secondarydex/expected.txt index 1c8defb6ec..1c8defb6ec 100644 --- a/test/127-secondarydex/expected.txt +++ b/test/127-checker-secondarydex/expected.txt diff --git a/test/127-secondarydex/info.txt b/test/127-checker-secondarydex/info.txt index 0479d1a784..0479d1a784 100644 --- a/test/127-secondarydex/info.txt +++ b/test/127-checker-secondarydex/info.txt diff --git a/test/127-secondarydex/run b/test/127-checker-secondarydex/run index d8c3c798bf..d8c3c798bf 100755 --- a/test/127-secondarydex/run +++ b/test/127-checker-secondarydex/run diff --git a/test/127-secondarydex/src/Main.java b/test/127-checker-secondarydex/src/Main.java index 0ede8ed2b2..0ede8ed2b2 100644 --- a/test/127-secondarydex/src/Main.java +++ b/test/127-checker-secondarydex/src/Main.java diff --git a/test/127-secondarydex/src/Super.java b/test/127-checker-secondarydex/src/Super.java index 7608d4a7c8..7608d4a7c8 100644 --- a/test/127-secondarydex/src/Super.java +++ b/test/127-checker-secondarydex/src/Super.java diff --git a/test/127-secondarydex/src/Test.java b/test/127-checker-secondarydex/src/Test.java index 8547e791c2..266ed191bc 100644 --- a/test/127-secondarydex/src/Test.java +++ b/test/127-checker-secondarydex/src/Test.java @@ -23,6 +23,13 @@ public class Test extends Super { System.out.println("Test"); } + /// CHECK-START: java.lang.Integer Test.toInteger() ssa_builder (after) + /// CHECK: LoadClass needs_access_check:false klass:java.lang.Integer + + public Integer toInteger() { + return new Integer(42); + } + public String toString() { return new String("Test"); } diff --git a/test/563-checker-fakestring/smali/TestCase.smali b/test/563-checker-fakestring/smali/TestCase.smali index cd52f3d55d..4bd804da26 100644 --- a/test/563-checker-fakestring/smali/TestCase.smali +++ b/test/563-checker-fakestring/smali/TestCase.smali @@ -64,9 +64,15 @@ .end method -# Test deoptimization between String's allocation and initialization. +# Test deoptimization between String's allocation and initialization. When not +# compiling --debuggable, the NewInstance will be optimized out. ## CHECK-START: int TestCase.deoptimizeNewInstance(int[], byte[]) register (after) +## CHECK: <<Null:l\d+>> NullConstant +## CHECK: Deoptimize env:[[<<Null>>,{{.*]]}} +## CHECK: InvokeStaticOrDirect method_name:java.lang.String.<init> + +## CHECK-START-DEBUGGABLE: int TestCase.deoptimizeNewInstance(int[], byte[]) register (after) ## CHECK: <<String:l\d+>> NewInstance ## CHECK: Deoptimize env:[[<<String>>,{{.*]]}} ## CHECK: InvokeStaticOrDirect method_name:java.lang.String.<init> @@ -98,3 +104,23 @@ return v2 .end method + +# Test that a redundant NewInstance is removed if not used and not compiling +# --debuggable. + +## CHECK-START: java.lang.String TestCase.removeNewInstance(byte[]) register (after) +## CHECK-NOT: NewInstance +## CHECK-NOT: LoadClass + +## CHECK-START-DEBUGGABLE: java.lang.String TestCase.removeNewInstance(byte[]) register (after) +## CHECK: NewInstance + +.method public static removeNewInstance([B)Ljava/lang/String; + .registers 5 + + new-instance v0, Ljava/lang/String; + const-string v1, "UTF8" + invoke-direct {v0, p0, v1}, Ljava/lang/String;-><init>([BLjava/lang/String;)V + return-object v0 + +.end method diff --git a/test/563-checker-fakestring/src/Main.java b/test/563-checker-fakestring/src/Main.java index 750f7184ee..04df0f637c 100644 --- a/test/563-checker-fakestring/src/Main.java +++ b/test/563-checker-fakestring/src/Main.java @@ -57,5 +57,11 @@ public class Main { } } } + + { + Method m = c.getMethod("removeNewInstance", byte[].class); + String result = (String) m.invoke(null, new Object[] { testData }); + assertEqual(testString, result); + } } } |