From c449e8b79aaaf156ce055524c41474cc1200ed5a Mon Sep 17 00:00:00 2001 From: Igor Murashkin Date: Wed, 10 Jun 2015 15:56:42 -0700 Subject: [PATCH] runtime: Minor cleanup and extra comments around interpreter Change-Id: I24c0b261de8cf737babd9d01bf679482d48c8bc9 --- compiler/dex/verified_method.cc | 2 +- compiler/dex/verified_method.h | 2 +- runtime/dex_file.h | 17 +++++++++++------ runtime/interpreter/interpreter_common.cc | 5 ++++- runtime/interpreter/interpreter_switch_impl.cc | 2 +- runtime/mirror/class-inl.h | 2 +- runtime/mirror/string-inl.h | 8 +++++--- runtime/mirror/string.h | 2 +- runtime/read_barrier-inl.h | 2 +- runtime/runtime_options.def | 13 ++++++++----- runtime/stack.h | 16 ++++++++++++++++ 11 files changed, 50 insertions(+), 21 deletions(-) diff --git a/compiler/dex/verified_method.cc b/compiler/dex/verified_method.cc index ac7a4a775..6d485986f 100644 --- a/compiler/dex/verified_method.cc +++ b/compiler/dex/verified_method.cc @@ -98,7 +98,7 @@ bool VerifiedMethod::GenerateGcMap(verifier::MethodVerifier* method_verifier) { } size_t ref_bitmap_bytes = RoundUp(ref_bitmap_bits, kBitsPerByte) / kBitsPerByte; // There are 2 bytes to encode the number of entries. - if (num_entries >= 65536) { + if (num_entries > std::numeric_limits::max()) { LOG(WARNING) << "Cannot encode GC map for method with " << num_entries << " entries: " << PrettyMethod(method_verifier->GetMethodReference().dex_method_index, *method_verifier->GetMethodReference().dex_file); diff --git a/compiler/dex/verified_method.h b/compiler/dex/verified_method.h index 242e3dfe6..07f9a9bd9 100644 --- a/compiler/dex/verified_method.h +++ b/compiler/dex/verified_method.h @@ -120,7 +120,7 @@ class VerifiedMethod { DequickenMap dequicken_map_; SafeCastSet safe_cast_set_; - bool has_verification_failures_; + bool has_verification_failures_ = false; // Copy of mapping generated by verifier of dex PCs of string init invocations // to the set of other registers that the receiver has been copied into. diff --git a/runtime/dex_file.h b/runtime/dex_file.h index d01760156..7ac264a0c 100644 --- a/runtime/dex_file.h +++ b/runtime/dex_file.h @@ -264,13 +264,18 @@ class DexFile { // Raw code_item. struct CodeItem { - uint16_t registers_size_; - uint16_t ins_size_; - uint16_t outs_size_; - uint16_t tries_size_; - uint32_t debug_info_off_; // file offset to debug info stream + uint16_t registers_size_; // the number of registers used by this code + // (locals + parameters) + uint16_t ins_size_; // the number of words of incoming arguments to the method + // that this code is for + uint16_t outs_size_; // the number of words of outgoing argument space required + // by this code for method invocation + uint16_t tries_size_; // the number of try_items for this instance. If non-zero, + // then these appear as the tries array just after the + // insns in this instance. + uint32_t debug_info_off_; // file offset to debug info stream uint32_t insns_size_in_code_units_; // size of the insns array, in 2 byte code units - uint16_t insns_[1]; + uint16_t insns_[1]; // actual array of bytecode. private: DISALLOW_COPY_AND_ASSIGN(CodeItem); diff --git a/runtime/interpreter/interpreter_common.cc b/runtime/interpreter/interpreter_common.cc index 86a79cead..0f6f78801 100644 --- a/runtime/interpreter/interpreter_common.cc +++ b/runtime/interpreter/interpreter_common.cc @@ -450,10 +450,13 @@ void UnexpectedOpcode(const Instruction* inst, const ShadowFrame& shadow_frame) static inline void AssignRegister(ShadowFrame* new_shadow_frame, const ShadowFrame& shadow_frame, size_t dest_reg, size_t src_reg) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - // If both register locations contains the same value, the register probably holds a reference. // Uint required, so that sign extension does not make this wrong on 64b systems uint32_t src_value = shadow_frame.GetVReg(src_reg); mirror::Object* o = shadow_frame.GetVRegReference(src_reg); + + // If both register locations contains the same value, the register probably holds a reference. + // Note: As an optimization, non-moving collectors leave a stale reference value + // in the references array even after the original vreg was overwritten to a non-reference. if (src_value == reinterpret_cast(o)) { new_shadow_frame->SetVRegReference(dest_reg, o); } else { diff --git a/runtime/interpreter/interpreter_switch_impl.cc b/runtime/interpreter/interpreter_switch_impl.cc index dd7aa4036..fcf083cbe 100644 --- a/runtime/interpreter/interpreter_switch_impl.cc +++ b/runtime/interpreter/interpreter_switch_impl.cc @@ -56,7 +56,7 @@ namespace interpreter { template JValue ExecuteSwitchImpl(Thread* self, const DexFile::CodeItem* code_item, ShadowFrame& shadow_frame, JValue result_register) { - bool do_assignability_check = do_access_check; + constexpr bool do_assignability_check = do_access_check; if (UNLIKELY(!shadow_frame.HasReferenceArray())) { LOG(FATAL) << "Invalid shadow frame for interpreter use"; return JValue(); diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h index 835b94ade..8c9222f6a 100644 --- a/runtime/mirror/class-inl.h +++ b/runtime/mirror/class-inl.h @@ -666,7 +666,7 @@ template inline void Class::VisitReferences(mirror::Class* klass, const Visitor& visitor) { VisitInstanceFieldsReferences(klass, visitor); // Right after a class is allocated, but not yet loaded - // (kStatusNotReady, see ClassLinkder::LoadClass()), GC may find it + // (kStatusNotReady, see ClassLinker::LoadClass()), GC may find it // and scan it. IsTemp() may call Class::GetAccessFlags() but may // fail in the DCHECK in Class::GetAccessFlags() because the class // status is kStatusNotReady. To avoid it, rely on IsResolved() diff --git a/runtime/mirror/string-inl.h b/runtime/mirror/string-inl.h index 9f6cd11c3..8d9c08d9d 100644 --- a/runtime/mirror/string-inl.h +++ b/runtime/mirror/string-inl.h @@ -176,11 +176,13 @@ inline String* String::AllocFromByteArray(Thread* self, int32_t byte_length, } template -inline String* String::AllocFromCharArray(Thread* self, int32_t array_length, +inline String* String::AllocFromCharArray(Thread* self, int32_t count, Handle array, int32_t offset, gc::AllocatorType allocator_type) { - SetStringCountAndValueVisitorFromCharArray visitor(array_length, array, offset); - String* new_string = Alloc(self, array_length, allocator_type, visitor); + // It is a caller error to have a count less than the actual array's size. + DCHECK_GE(array->GetLength(), count); + SetStringCountAndValueVisitorFromCharArray visitor(count, array, offset); + String* new_string = Alloc(self, count, allocator_type, visitor); return new_string; } diff --git a/runtime/mirror/string.h b/runtime/mirror/string.h index a8f16d78f..af0638540 100644 --- a/runtime/mirror/string.h +++ b/runtime/mirror/string.h @@ -95,7 +95,7 @@ class MANAGED String FINAL : public Object { SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); template - ALWAYS_INLINE static String* AllocFromCharArray(Thread* self, int32_t array_length, + ALWAYS_INLINE static String* AllocFromCharArray(Thread* self, int32_t count, Handle array, int32_t offset, gc::AllocatorType allocator_type) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); diff --git a/runtime/read_barrier-inl.h b/runtime/read_barrier-inl.h index d341ee101..8d84c35bd 100644 --- a/runtime/read_barrier-inl.h +++ b/runtime/read_barrier-inl.h @@ -31,7 +31,7 @@ namespace art { template inline MirrorType* ReadBarrier::Barrier( mirror::Object* obj, MemberOffset offset, mirror::HeapReference* ref_addr) { - const bool with_read_barrier = kReadBarrierOption == kWithReadBarrier; + constexpr bool with_read_barrier = kReadBarrierOption == kWithReadBarrier; if (with_read_barrier && kUseBakerReadBarrier) { // The higher bits of the rb ptr, rb_ptr_high_bits (must be zero) // is used to create artificial data dependency from the is_gray diff --git a/runtime/runtime_options.def b/runtime/runtime_options.def index 922334ee9..4a307d5ed 100644 --- a/runtime/runtime_options.def +++ b/runtime/runtime_options.def @@ -30,6 +30,8 @@ // If a default value is omitted here, T{} is used as the default value, which is // almost-always the value of the type as if it was memset to all 0. // +// Please keep the columns aligned if possible when adding new rows. +// // Parse-able keys from the command line. RUNTIME_OPTIONS_KEY (Unit, Zygote) @@ -64,9 +66,9 @@ RUNTIME_OPTIONS_KEY (Unit, IgnoreMaxFootprint) RUNTIME_OPTIONS_KEY (Unit, LowMemoryMode) RUNTIME_OPTIONS_KEY (bool, UseTLAB, (kUseTlab || kUseReadBarrier)) RUNTIME_OPTIONS_KEY (bool, EnableHSpaceCompactForOOM, true) -RUNTIME_OPTIONS_KEY (bool, UseJIT, false) -RUNTIME_OPTIONS_KEY (unsigned int, JITCompileThreshold, jit::Jit::kDefaultCompileThreshold) -RUNTIME_OPTIONS_KEY (MemoryKiB, JITCodeCacheCapacity, jit::JitCodeCache::kDefaultCapacity) +RUNTIME_OPTIONS_KEY (bool, UseJIT, false) +RUNTIME_OPTIONS_KEY (unsigned int, JITCompileThreshold, jit::Jit::kDefaultCompileThreshold) +RUNTIME_OPTIONS_KEY (MemoryKiB, JITCodeCacheCapacity, jit::JitCodeCache::kDefaultCapacity) RUNTIME_OPTIONS_KEY (MillisecondsToNanoseconds, \ HSpaceCompactForOOMMinIntervalsMs,\ MsToNs(100 * 1000)) // 100s @@ -105,9 +107,12 @@ RUNTIME_OPTIONS_KEY (std::vector, \ ImageCompilerOptions) // -Ximage-compiler-option ... RUNTIME_OPTIONS_KEY (bool, Verify, true) RUNTIME_OPTIONS_KEY (std::string, NativeBridge) +RUNTIME_OPTIONS_KEY (unsigned int, ZygoteMaxFailedBoots, 10) +RUNTIME_OPTIONS_KEY (Unit, NoDexFileFallback) RUNTIME_OPTIONS_KEY (std::string, CpuAbiList) // Not parse-able from command line, but can be provided explicitly. +// (Do not add anything here that is defined in ParsedOptions::MakeParser) RUNTIME_OPTIONS_KEY (const std::vector*, \ BootClassPathDexList) // TODO: make unique_ptr RUNTIME_OPTIONS_KEY (InstructionSet, ImageInstructionSet, kRuntimeISA) @@ -120,7 +125,5 @@ RUNTIME_OPTIONS_KEY (void (*)(int32_t status), \ // We don't call abort(3) by default; see // Runtime::Abort. RUNTIME_OPTIONS_KEY (void (*)(), HookAbort, nullptr) -RUNTIME_OPTIONS_KEY (unsigned int, ZygoteMaxFailedBoots, 10) -RUNTIME_OPTIONS_KEY (Unit, NoDexFileFallback) #undef RUNTIME_OPTIONS_KEY diff --git a/runtime/stack.h b/runtime/stack.h index 79d2f40d7..d60714f7a 100644 --- a/runtime/stack.h +++ b/runtime/stack.h @@ -95,6 +95,8 @@ class ShadowFrame { } ~ShadowFrame() {} + // TODO(iam): Clean references array up since they're always there, + // we don't need to do conditionals. bool HasReferenceArray() const { return true; } @@ -149,6 +151,9 @@ class ShadowFrame { return *reinterpret_cast(vreg); } + // Look up the reference given its virtual register number. + // If this returns non-null then this does not mean the vreg is currently a reference + // on non-moving collectors. Check that the raw reg with GetVReg is equal to this if not certain. template mirror::Object* GetVRegReference(size_t i) const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { DCHECK_LT(i, NumberOfVRegs()); @@ -283,6 +288,8 @@ class ShadowFrame { ShadowFrame(uint32_t num_vregs, ShadowFrame* link, ArtMethod* method, uint32_t dex_pc, bool has_reference_array) : number_of_vregs_(num_vregs), link_(link), method_(method), dex_pc_(dex_pc) { + // TODO(iam): Remove this parameter, it's an an artifact of portable removal + DCHECK(has_reference_array); if (has_reference_array) { memset(vregs_, 0, num_vregs * (sizeof(uint32_t) + sizeof(StackReference))); } else { @@ -306,6 +313,15 @@ class ShadowFrame { ShadowFrame* link_; ArtMethod* method_; uint32_t dex_pc_; + + // This is a two-part array: + // - [0..number_of_vregs) holds the raw virtual registers, and each element here is always 4 + // bytes. + // - [number_of_vregs..number_of_vregs*2) holds only reference registers. Each element here is + // ptr-sized. + // In other words when a primitive is stored in vX, the second (reference) part of the array will + // be null. When a reference is stored in vX, the second (reference) part of the array will be a + // copy of vX. uint32_t vregs_[0]; DISALLOW_IMPLICIT_CONSTRUCTORS(ShadowFrame); -- 2.11.0