src: boxed double fields for V8 8.1 · nodejs/llnode@dd57bfb · GitHub
Skip to content

Commit dd57bfb

Browse files
mmarchinimmarchini
committed
src: boxed double fields for V8 8.1
V8 8.1 disabled unboxed double fieds, which means all double fields are now stored on memory as references to HeapNumber objects instead of the double value. In theory V8 could store boxed doubles before, but we didn't take it into account in our dobule fields code. In the future, V8 intends to re-enable unboxed doubles, which means we'll need to add logic to handle both types of fields in the same llnode session. This also fixes a bug where llnode was not handling double fields at all in some cases. Fixes: #353 Signed-off-by: Matheus Marchini <mmarchini@netflix.com> PR-URL: #358 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent fdddce0 commit dd57bfb

7 files changed

Lines changed: 102 additions & 23 deletions

File tree

‎src/error.h‎

Lines changed: 6 additions & 0 deletions

‎src/llv8-constants.cc‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,24 @@ void Map::Load() {
128128
kNumberOfOwnDescriptorsMask = (1 << kDescriptorIndexBitCount) - 1;
129129
kNumberOfOwnDescriptorsMask <<= kNumberOfOwnDescriptorsShift;
130130
}
131+
kLayoutDescriptor =
132+
LoadConstant({"class_Map__layout_descriptor__LayoutDescriptor"});
131133
}
132134

133135

136+
bool Map::HasUnboxedDoubleFields() {
137+
// LayoutDescriptor is used by V8 to define which fields are not tagged
138+
// (unboxed). In preparation for pointer compressions, V8 disabled unboxed
139+
// doubles everywhere, which means Map doesn't have a layout_descriptor
140+
// field, but because of how gen-postmortem-metadata works and how Torque
141+
// generates the offsets file, we get a constant for it anyway. In the future
142+
// unboxing will be enabled again, in which case this field will be used.
143+
// Until then, we use the presence of this field as version (if the field is
144+
// present, it's safe to assume we're on V8 8.1+, at least on supported
145+
// release lines).
146+
return !kLayoutDescriptor.Loaded();
147+
}
148+
134149
void JSObject::Load() {
135150
kPropertiesOffset =
136151
LoadConstant("class_JSReceiver__raw_properties_or_hash__Object",

‎src/llv8-constants.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,14 @@ class Map : public Module {
7777
int64_t kInObjectPropertiesStartOffset;
7878
int64_t kInstanceSizeOffset;
7979
int64_t kInstanceTypeOffset;
80+
Constant<int64_t> kLayoutDescriptor;
8081

8182
int64_t kNumberOfOwnDescriptorsMask;
8283
int64_t kNumberOfOwnDescriptorsShift;
8384
int64_t kDictionaryMapShift;
8485

86+
bool HasUnboxedDoubleFields();
87+
8588
protected:
8689
void Load();
8790
};

‎src/llv8-inl.h‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ inline bool HeapObject::Check() const {
8989
(raw() & v8()->heap_obj()->kTagMask) == v8()->heap_obj()->kTag;
9090
}
9191

92+
inline bool HeapNumber::Check() const {
93+
if (unboxed_double_) return Value::Check();
94+
return HeapObject::Check();
95+
}
9296

9397
int64_t HeapObject::LeaField(int64_t off) const {
9498
return raw() - v8()->heap_obj()->kTag + off;
@@ -388,8 +392,46 @@ inline T JSObject::GetInObjectValue(int64_t size, int index, Error& err) {
388392
return LoadFieldValue<T>(size + index * v8()->common()->kPointerSize, err);
389393
}
390394

395+
inline HeapNumber JSObject::GetDoubleField(int64_t index, Error err) {
396+
HeapObject map_obj = GetMap(err);
397+
if (err.Fail()) HeapNumber();
398+
399+
Map map(map_obj);
400+
int64_t instance_size = map.InstanceSize(err);
401+
if (err.Fail()) return HeapNumber();
402+
403+
// TODO(mmarchini): Explain why index might be lower than zero.
404+
if (index < 0) {
405+
// When unboxed doubles are not allowed, all double fields are stored as
406+
// HeapNumber objects.
407+
if (v8()->map()->HasUnboxedDoubleFields()) {
408+
// TODO(mmarchini): GetInObjectValue call chain should be
409+
// CheckedType-aware instead of relying on Error.
410+
Error get_in_object_value_err;
411+
double result = GetInObjectValue<double>(instance_size, index,
412+
get_in_object_value_err);
413+
if (get_in_object_value_err.Fail()) {
414+
return HeapNumber(v8(), CheckedType<double>());
415+
} else {
416+
return HeapNumber(v8(), CheckedType<double>(result));
417+
}
418+
}
419+
return GetInObjectValue<HeapNumber>(instance_size, index, err);
420+
}
421+
HeapObject extra_properties_obj = Properties(err);
422+
if (err.Fail()) return HeapNumber();
423+
424+
FixedArray extra_properties(extra_properties_obj);
425+
426+
return HeapNumber(v8(), extra_properties.Get<double>(index, err));
427+
}
428+
429+
inline const CheckedType<double> HeapNumber::GetValue(Error& err) {
430+
if (unboxed_double_) return unboxed_value_;
431+
return GetHeapNumberValue(err);
432+
};
391433

392-
ACCESSOR(HeapNumber, GetValue, heap_number()->kValueOffset, double)
434+
ACCESSOR(HeapNumber, GetHeapNumberValue, heap_number()->kValueOffset, double)
393435

394436
ACCESSOR(JSArray, Length, js_array()->kLengthOffset, Smi)
395437

‎src/llv8.cc‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -693,8 +693,12 @@ std::string HeapObject::GetTypeName(Error& err) {
693693
std::string HeapNumber::ToString(bool whole, Error& err) {
694694
char buf[128];
695695
const char* fmt = whole ? "%f" : "%.2f";
696-
snprintf(buf, sizeof(buf), fmt, GetValue(err));
697-
err = Error::Ok();
696+
auto val = GetValue(err);
697+
if (err.Fail() || !val.Check()) {
698+
err = Error::Ok();
699+
return "???";
700+
}
701+
snprintf(buf, sizeof(buf), fmt, val);
698702
return buf;
699703
}
700704

@@ -1176,15 +1180,7 @@ Value JSObject::GetDescriptorProperty(std::string key_name, Map map,
11761180
int64_t index = descriptors.FieldIndex(details) - in_object_count;
11771181

11781182
if (descriptors.IsDoubleField(details)) {
1179-
double value;
1180-
if (index < 0) {
1181-
value = GetInObjectValue<double>(instance_size, index, err);
1182-
} else {
1183-
value = extra_properties.Get<double>(index, err);
1184-
}
1185-
1186-
if (err.Fail()) return Value();
1187-
1183+
return GetDoubleField(index, err);
11881184
} else {
11891185
Value value;
11901186
if (index < 0) {

‎src/llv8.h‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,13 +277,35 @@ class ThinString : public String {
277277
inline std::string ToString(Error& err);
278278
};
279279

280+
// Numbers on the heap can be a boxed HeapNumber or a unboxed double.
280281
class HeapNumber : public HeapObject {
281282
public:
282283
V8_VALUE_DEFAULT_METHODS(HeapNumber, HeapObject)
283284

284-
inline double GetValue(Error& err);
285+
HeapNumber(LLV8* v8, CheckedType<double> value)
286+
: HeapObject(v8, 0), unboxed_value_(value), unboxed_double_(true) {}
287+
288+
inline bool Check() const;
289+
290+
inline int64_t raw() const {
291+
if (unboxed_double_) {
292+
PRINT_DEBUG("Shouldn't try to access raw() of unboxed double");
293+
#if DEBUG
294+
unreachable();
295+
#endif
296+
}
297+
return HeapObject::raw();
298+
}
299+
300+
inline const CheckedType<double> GetValue(Error& err);
285301

286302
std::string ToString(bool whole, Error& err);
303+
304+
private:
305+
inline double GetHeapNumberValue(Error& err);
306+
307+
const CheckedType<double> unboxed_value_;
308+
const bool unboxed_double_ = false;
287309
};
288310

289311
class JSObject : public HeapObject {
@@ -309,6 +331,8 @@ class JSObject : public HeapObject {
309331

310332
static inline bool IsObjectType(LLV8* v8, int64_t type);
311333

334+
inline HeapNumber GetDoubleField(int64_t index, Error err);
335+
312336
protected:
313337
friend class llnode::Printer;
314338
template <class T>

‎src/printer.cc‎

Lines changed: 3 additions & 10 deletions

0 commit comments

Comments
 (0)