From 55d02cf056f993aeafebd54e7b7c68c7a48507c9 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Thu, 29 Oct 2015 02:59:50 +0000 Subject: [PATCH] Revert "Enable store elimination for singleton objects." This reverts commit 7f43a3d48fc29045875d50e10bbc5d6ffc25d61e. Fails booting. Bug: 25357772 Change-Id: Ied19536f3ce8d81e76885cb6baed4853e2ed6714 --- compiler/dex/quick/gen_common.cc | 6 +- compiler/driver/compiler_driver.cc | 6 +- compiler/driver/compiler_driver.h | 6 +- compiler/optimizing/builder.cc | 16 ++--- compiler/optimizing/builder.h | 2 +- compiler/optimizing/load_store_elimination.cc | 12 +--- compiler/optimizing/nodes.h | 16 ++--- test/530-checker-lse/src/Main.java | 85 ++++----------------------- 8 files changed, 29 insertions(+), 120 deletions(-) diff --git a/compiler/dex/quick/gen_common.cc b/compiler/dex/quick/gen_common.cc index 5da72147b..2b60a51e2 100644 --- a/compiler/dex/quick/gen_common.cc +++ b/compiler/dex/quick/gen_common.cc @@ -1104,11 +1104,7 @@ void Mir2Lir::GenNewInstance(uint32_t type_idx, RegLocation rl_dest) { // access because the verifier was unable to? const DexFile* dex_file = cu_->dex_file; CompilerDriver* driver = cu_->compiler_driver; - bool finalizable; - if (driver->CanAccessInstantiableTypeWithoutChecks(cu_->method_idx, - *dex_file, - type_idx, - &finalizable)) { + if (driver->CanAccessInstantiableTypeWithoutChecks(cu_->method_idx, *dex_file, type_idx)) { bool is_type_initialized; bool use_direct_type_ptr; uintptr_t direct_type_ptr; diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index fa3598e29..8750aa8e4 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -1247,8 +1247,7 @@ bool CompilerDriver::CanAccessTypeWithoutChecks(uint32_t referrer_idx, const Dex bool CompilerDriver::CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_idx, const DexFile& dex_file, - uint32_t type_idx, - bool* finalizable) { + uint32_t type_idx) { ScopedObjectAccess soa(Thread::Current()); mirror::DexCache* dex_cache = Runtime::Current()->GetClassLinker()->FindDexCache( soa.Self(), dex_file, false); @@ -1256,11 +1255,8 @@ bool CompilerDriver::CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_id mirror::Class* resolved_class = dex_cache->GetResolvedType(type_idx); if (resolved_class == nullptr) { stats_->TypeNeedsAccessCheck(); - // Be conservative. - *finalizable = true; return false; // Unknown class needs access checks. } - *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) { diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h index 15806b557..485cdcfb1 100644 --- a/compiler/driver/compiler_driver.h +++ b/compiler/driver/compiler_driver.h @@ -200,10 +200,8 @@ class CompilerDriver { REQUIRES(!Locks::mutator_lock_); // Are runtime access and instantiable checks necessary in the code? - bool CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_idx, - const DexFile& dex_file, - uint32_t type_idx, - bool* finalizable) + bool CanAccessInstantiableTypeWithoutChecks(uint32_t referrer_idx, const DexFile& dex_file, + uint32_t type_idx) REQUIRES(!Locks::mutator_lock_); bool CanEmbedTypeInCode(const DexFile& dex_file, uint32_t type_idx, diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc index ee5b92987..ed193c7b6 100644 --- a/compiler/optimizing/builder.cc +++ b/compiler/optimizing/builder.cc @@ -1455,8 +1455,7 @@ void HGraphBuilder::BuildFilledNewArray(uint32_t dex_pc, uint32_t* args, uint32_t register_index) { HInstruction* length = graph_->GetIntConstant(number_of_vreg_arguments, dex_pc); - bool finalizable; - QuickEntrypointEnum entrypoint = NeedsAccessCheck(type_index, &finalizable) + QuickEntrypointEnum entrypoint = NeedsAccessCheck(type_index) ? kQuickAllocArrayWithAccessCheck : kQuickAllocArray; HInstruction* object = new (arena_) HNewArray(length, @@ -1636,9 +1635,9 @@ void HGraphBuilder::BuildTypeCheck(const Instruction& instruction, } } -bool HGraphBuilder::NeedsAccessCheck(uint32_t type_index, bool* finalizable) const { +bool HGraphBuilder::NeedsAccessCheck(uint32_t type_index) const { return !compiler_driver_->CanAccessInstantiableTypeWithoutChecks( - dex_compilation_unit_->GetDexMethodIndex(), *dex_file_, type_index, finalizable); + dex_compilation_unit_->GetDexMethodIndex(), *dex_file_, type_index); } void HGraphBuilder::BuildSwitchJumpTable(const SwitchTable& table, @@ -2515,9 +2514,7 @@ bool HGraphBuilder::AnalyzeDexInstruction(const Instruction& instruction, uint32 current_block_->AddInstruction(fake_string); UpdateLocal(register_index, fake_string, dex_pc); } else { - bool finalizable; - bool can_throw = NeedsAccessCheck(type_index, &finalizable); - QuickEntrypointEnum entrypoint = can_throw + QuickEntrypointEnum entrypoint = NeedsAccessCheck(type_index) ? kQuickAllocObjectWithAccessCheck : kQuickAllocObject; @@ -2526,8 +2523,6 @@ bool HGraphBuilder::AnalyzeDexInstruction(const Instruction& instruction, uint32 dex_pc, type_index, *dex_compilation_unit_->GetDexFile(), - can_throw, - finalizable, entrypoint)); UpdateLocal(instruction.VRegA(), current_block_->GetLastInstruction(), dex_pc); } @@ -2537,8 +2532,7 @@ bool HGraphBuilder::AnalyzeDexInstruction(const Instruction& instruction, uint32 case Instruction::NEW_ARRAY: { uint16_t type_index = instruction.VRegC_22c(); HInstruction* length = LoadLocal(instruction.VRegB_22c(), Primitive::kPrimInt, dex_pc); - bool finalizable; - QuickEntrypointEnum entrypoint = NeedsAccessCheck(type_index, &finalizable) + QuickEntrypointEnum entrypoint = NeedsAccessCheck(type_index) ? kQuickAllocArrayWithAccessCheck : kQuickAllocArray; current_block_->AddInstruction(new (arena_) HNewArray(length, diff --git a/compiler/optimizing/builder.h b/compiler/optimizing/builder.h index 0f64489d9..9eaa4b62c 100644 --- a/compiler/optimizing/builder.h +++ b/compiler/optimizing/builder.h @@ -138,7 +138,7 @@ class HGraphBuilder : public ValueObject { HInstruction* LoadLocal(uint32_t register_index, Primitive::Type type, uint32_t dex_pc) const; void PotentiallyAddSuspendCheck(HBasicBlock* target, uint32_t dex_pc); void InitializeParameters(uint16_t number_of_parameters); - bool NeedsAccessCheck(uint32_t type_index, bool* finalizable) const; + bool NeedsAccessCheck(uint32_t type_index) const; template void Unop_12x(const Instruction& instruction, Primitive::Type type, uint32_t dex_pc); diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc index aa9c315dd..90f28e511 100644 --- a/compiler/optimizing/load_store_elimination.cc +++ b/compiler/optimizing/load_store_elimination.cc @@ -695,12 +695,8 @@ class LSEVisitor : public HGraphVisitor { } else { redundant_store = true; } - HNewInstance* new_instance = ref_info->GetReference()->AsNewInstance(); - DCHECK(new_instance != nullptr); - if (new_instance->IsFinalizable()) { - // Finalizable objects escape globally. Need to keep the store. - redundant_store = false; - } + // TODO: eliminate the store if the singleton object is not finalizable. + redundant_store = false; } if (redundant_store) { removed_instructions_.push_back(instruction); @@ -838,9 +834,7 @@ class LSEVisitor : public HGraphVisitor { return; } if (!heap_location_collector_.MayDeoptimize() && - ref_info->IsSingletonAndNotReturned() && - !new_instance->IsFinalizable() && - !new_instance->CanThrow()) { + ref_info->IsSingletonAndNotReturned()) { // The allocation might be eliminated. singleton_new_instances_.push_back(new_instance); } diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 7ac39d1a8..6028d4b6f 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -3595,14 +3595,10 @@ class HNewInstance : public HExpression<1> { uint32_t dex_pc, uint16_t type_index, const DexFile& dex_file, - bool can_throw, - bool finalizable, QuickEntrypointEnum entrypoint) : HExpression(Primitive::kPrimNot, SideEffects::CanTriggerGC(), dex_pc), type_index_(type_index), dex_file_(dex_file), - can_throw_(can_throw), - finalizable_(finalizable), entrypoint_(entrypoint) { SetRawInputAt(0, current_method); } @@ -3612,11 +3608,11 @@ class HNewInstance : public HExpression<1> { // Calls runtime so needs an environment. bool NeedsEnvironment() const OVERRIDE { return true; } - - // It may throw when called on type that's not instantiable/accessible. - bool CanThrow() const OVERRIDE { return can_throw_; } - - bool IsFinalizable() const { return finalizable_; } + // It may throw when called on: + // - interfaces + // - abstract/innaccessible/unknown classes + // TODO: optimize when possible. + bool CanThrow() const OVERRIDE { return true; } bool CanBeNull() const OVERRIDE { return false; } @@ -3627,8 +3623,6 @@ class HNewInstance : public HExpression<1> { private: const uint16_t type_index_; const DexFile& dex_file_; - const bool can_throw_; - const bool finalizable_; const QuickEntrypointEnum entrypoint_; DISALLOW_COPY_AND_ASSIGN(HNewInstance); diff --git a/test/530-checker-lse/src/Main.java b/test/530-checker-lse/src/Main.java index 924ff6773..c766aaa6c 100644 --- a/test/530-checker-lse/src/Main.java +++ b/test/530-checker-lse/src/Main.java @@ -22,7 +22,7 @@ class Circle { return radius * radius * Math.PI; } private double radius; -} +}; class TestClass { TestClass() { @@ -36,29 +36,16 @@ class TestClass { volatile int k; TestClass next; static int si; -} +}; class SubTestClass extends TestClass { int k; -} +}; class TestClass2 { int i; int j; -} - -class Finalizable { - static boolean sVisited = false; - static final int VALUE = 0xbeef; - int i; - - protected void finalize() { - if (i != VALUE) { - System.out.println("Where is the beef?"); - } - sVisited = true; - } -} +}; public class Main { @@ -69,7 +56,7 @@ public class Main { /// CHECK-START: double Main.calcCircleArea(double) load_store_elimination (after) /// CHECK: NewInstance - /// CHECK-NOT: InstanceFieldSet + /// CHECK: InstanceFieldSet /// CHECK-NOT: InstanceFieldGet static double calcCircleArea(double radius) { @@ -130,7 +117,7 @@ public class Main { /// CHECK: InstanceFieldGet /// CHECK: InstanceFieldSet /// CHECK: NewInstance - /// CHECK-NOT: InstanceFieldSet + /// CHECK: InstanceFieldSet /// CHECK-NOT: InstanceFieldGet // A new allocation shouldn't alias with pre-existing values. @@ -236,7 +223,7 @@ public class Main { /// CHECK-START: int Main.test8() load_store_elimination (after) /// CHECK: NewInstance - /// CHECK-NOT: InstanceFieldSet + /// CHECK: InstanceFieldSet /// CHECK: InvokeVirtual /// CHECK-NOT: NullCheck /// CHECK-NOT: InstanceFieldGet @@ -394,8 +381,8 @@ public class Main { /// CHECK-START: int Main.test16() load_store_elimination (after) /// CHECK: NewInstance - /// CHECK-NOT: InstanceFieldSet - /// CHECK-NOT: InstanceFieldGet + /// CHECK-NOT: StaticFieldSet + /// CHECK-NOT: StaticFieldGet // Test inlined constructor. static int test16() { @@ -411,8 +398,8 @@ public class Main { /// CHECK-START: int Main.test17() load_store_elimination (after) /// CHECK: <> IntConstant 0 /// CHECK: NewInstance - /// CHECK-NOT: InstanceFieldSet - /// CHECK-NOT: InstanceFieldGet + /// CHECK-NOT: StaticFieldSet + /// CHECK-NOT: StaticFieldGet /// CHECK: Return [<>] // Test getting default value. @@ -468,55 +455,6 @@ public class Main { return obj; } - /// CHECK-START: void Main.testFinalizable() load_store_elimination (before) - /// CHECK: NewInstance - /// CHECK: InstanceFieldSet - - /// CHECK-START: void Main.testFinalizable() load_store_elimination (after) - /// CHECK: NewInstance - /// CHECK: InstanceFieldSet - - // Allocations and stores into finalizable objects cannot be eliminated. - static void testFinalizable() { - Finalizable finalizable = new Finalizable(); - finalizable.i = Finalizable.VALUE; - } - - static java.lang.ref.WeakReference getWeakReference() { - return new java.lang.ref.WeakReference<>(new Object()); - } - - static void testFinalizableByForcingGc() { - testFinalizable(); - java.lang.ref.WeakReference reference = getWeakReference(); - - Runtime runtime = Runtime.getRuntime(); - for (int i = 0; i < 20; ++i) { - runtime.gc(); - System.runFinalization(); - try { - Thread.sleep(1); - } catch (InterruptedException e) { - throw new AssertionError(e); - } - - // Check to see if the weak reference has been garbage collected. - if (reference.get() == null) { - // A little bit more sleep time to make sure. - try { - Thread.sleep(100); - } catch (InterruptedException e) { - throw new AssertionError(e); - } - if (!Finalizable.sVisited) { - System.out.println("finalize() not called."); - } - return; - } - } - System.out.println("testFinalizableByForcingGc() failed to force gc."); - } - public static void assertIntEquals(int expected, int result) { if (expected != result) { throw new Error("Expected: " + expected + ", found: " + result); @@ -570,6 +508,5 @@ public class Main { float[] fa2 = { 1.8f }; assertFloatEquals(test19(fa1, fa2), 1.8f); assertFloatEquals(test20().i, 0); - testFinalizableByForcingGc(); } } -- 2.11.0