[tools][runtime] Fix --trace-maps

- Don't print normalize transition for cached maps
- Avoid printing two transitions in Map::CopyReplaceDescriptor
- Harden processor.mjs existing existing broken logs by skipping double
  entries and avoiding mutliple edges to the same target map

Bug: v8:10644
Change-Id: I561a0f888c8835a40a289baa50d65ff69e368bad
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565123Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71565}

src/objects/map.cc

@@ -1520,6 +1520,9 @@ Handle<Map> Map::Normalize(Isolate* isolate, Handle<Map> fast_map,
                           Map::kSize - offset));
     }
 #endif
+    if (FLAG_trace_maps) {
+      LOG(isolate, MapEvent("NormalizeCached", fast_map, new_map, reason));
+    }
   } else {
     new_map = Map::CopyNormalized(isolate, fast_map, mode);
     new_map->set_elements_kind(new_elements_kind);
@@ -1527,9 +1530,9 @@ Handle<Map> Map::Normalize(Isolate* isolate, Handle<Map> fast_map,
       cache->Set(fast_map, new_map);
       isolate->counters()->maps_normalized()->Increment();
     }
-  }
-  if (FLAG_trace_maps) {
-    LOG(isolate, MapEvent("Normalize", fast_map, new_map, reason));
+    if (FLAG_trace_maps) {
+      LOG(isolate, MapEvent("Normalize", fast_map, new_map, reason));
+    }
   }
   fast_map->NotifyLeafMapLayoutChange(isolate);
   return new_map;
@@ -1732,6 +1735,7 @@ Handle<Map> Map::CopyReplaceDescriptors(
   DCHECK(descriptors->IsSortedNoDuplicates());
 
   Handle<Map> result = CopyDropDescriptors(isolate, map);
+  bool is_connected = false;
 
   // Properly mark the {result} if the {name} is an "interesting symbol".
   Handle<Name> name;
@@ -1748,17 +1752,14 @@ Handle<Map> Map::CopyReplaceDescriptors(
 
       DCHECK(!maybe_name.is_null());
       ConnectTransition(isolate, map, result, name, simple_flag);
+      is_connected = true;
     } else {
       descriptors->GeneralizeAllFields();
       result->InitializeDescriptors(isolate, *descriptors,
                                     LayoutDescriptor::FastPointerLayout());
     }
   }
-  if (FLAG_trace_maps &&
-      // Mirror conditions above that did not call ConnectTransition().
-      (map->IsDetached(isolate) ||
-       !(flag == INSERT_TRANSITION &&
-         TransitionsAccessor(isolate, map).CanHaveMoreTransitions()))) {
+  if (FLAG_trace_maps && !is_connected) {
     LOG(isolate, MapEvent("ReplaceDescriptors", map, result, reason,
                           maybe_name.is_null() ? Handle<HeapObject>() : name));
   }

tools/system-analyzer/log/map.mjs

@@ -34,6 +34,7 @@ define(Array.prototype, 'last', function() {
 // Map Log Events
 
 class MapLogEntry extends LogEntry {
+  id = -1;
   edge = undefined;
   children = [];
   depth = 0;
@@ -43,13 +44,14 @@ class MapLogEntry extends LogEntry {
   rightId = 0;
   filePosition = '';
   script = '';
-  id = -1;
   description = '';
   constructor(id, time) {
     if (!time) throw new Error('Invalid time');
-    super(id, time);
-    MapLogEntry.set(id, this);
+    // Use MapLogEntry.type getter instead of property, since we only know the
+    // type lazily from the incoming transition.
+    super(undefined, time);
     this.id = id;
+    MapLogEntry.set(id, this);
   }
 
   toString() {
@@ -64,9 +66,9 @@ class MapLogEntry extends LogEntry {
         console.warn('Skipping potential parent loop between maps:', current)
         continue;
       }
-      current.finalize(id)
+      current.finalize(id);
       id += 1;
-      current.children.forEach(edge => stack.push(edge.to))
+      current.children.forEach(edge => stack.push(edge.to));
       // TODO implement rightId
     }
     return id;
@@ -155,15 +157,16 @@ class MapLogEntry extends LogEntry {
 
   static get(id, time = undefined) {
     let maps = this.cache.get(id);
-    if (maps) {
+    if (maps === undefined) return undefined;
+    if (time !== undefined) {
       for (let i = 1; i < maps.length; i++) {
         if (maps[i].time > time) {
           return maps[i - 1];
         }
       }
-      // default return the latest
-      return (maps.length > 0) ? maps[maps.length - 1] : undefined;
     }
+    // default return the latest
+    return maps[maps.length - 1];
   }
 
   static set(id, map) {
@@ -188,17 +191,29 @@ class Edge {
     this.to = to;
   }
 
+  updateFrom(edge) {
+    if (this.to !== edge.to || this.from !== edge.from) {
+      throw new Error('Invalid Edge updated', this, to);
+    }
+    this.type = edge.type;
+    this.name = edge.name;
+    this.reason = edge.reason;
+    this.time = edge.time;
+  }
+
   finishSetup() {
     const from = this.from;
-    if (from) from.addEdge(this);
     const to = this.to;
+    if (to?.time < from?.time) {
+      // This happens for map deprecation where the transition tree is converted
+      // in reverse order.
+      console.warn('Invalid time order');
+    }
+    if (from) from.addEdge(this);
     if (to === undefined) return;
     to.edge = this;
     if (from === undefined) return;
     if (to === from) throw 'From and to must be distinct.';
-    if (to.time < from.time) {
-      console.warn('invalid time order');
-    }
     let newDepth = from.depth + 1;
     if (to.depth > 0 && to.depth != newDepth) {
       console.warn('Depth has already been initialized');

tools/system-analyzer/processor.mjs

@@ -284,10 +284,21 @@ export class Processor extends LogReader {
   }
 
   processMap(type, time, from, to, pc, line, column, reason, name) {
-    let time_ = parseInt(time);
+    const time_ = parseInt(time);
     if (type === 'Deprecate') return this.deprecateMap(type, time_, from);
-    let from_ = this.getExistingMapEntry(from, time_);
-    let to_ = this.getExistingMapEntry(to, time_);
+    // Skip normalized maps that were cached so we don't introduce multiple
+    // edges with the same source and target map.
+    if (type === 'NormalizeCached') return;
+    const from_ = this.getMapEntry(from, time_);
+    const to_ = this.getMapEntry(to, time_);
+    if (type === 'Normalize') {
+      // Fix a bug where we log "Normalize" transitions for maps created from
+      // the NormalizedMapCache.
+      if (to_.parent()?.id === from || to_.time < from_.time || to_.depth > 0) {
+        console.log(`Skipping transition to cached normalized map`);
+        return;
+      }
+    }
     // TODO: use SourcePosition directly.
     let edge = new Edge(type, name, reason, time, from_, to_);
     const profileEntry = this._profile.findEntry(pc)
@@ -297,40 +308,44 @@ export class Processor extends LogReader {
       to_.script = script;
       to_.sourcePosition = to_.script.addSourcePosition(line, column, to_)
     }
-    edge.finishSetup();
+    if (to_.parent() !== undefined && to_.parent() === from_) {
+      // Fix bug where we double log transitions.
+      console.warn('Fixing up double transition');
+      to_.edge.updateFrom(edge);
+    } else {
+      edge.finishSetup();
+    }
   }
 
   deprecateMap(type, time, id) {
-    this.getExistingMapEntry(id, time).deprecate();
+    this.getMapEntry(id, time).deprecate();
   }
 
   processMapCreate(time, id) {
     // map-create events might override existing maps if the addresses get
     // recycled. Hence we do not check for existing maps.
-    let map = this.createMapEntry(id, time);
+    this.createMapEntry(id, time);
   }
 
   processMapDetails(time, id, string) {
     // TODO(cbruni): fix initial map logging.
-    let map = this.getExistingMapEntry(id, time);
+    const map = this.getMapEntry(id, time);
     map.description = string;
   }
 
   createMapEntry(id, time) {
-    let map = new MapLogEntry(id, time);
+    const map = new MapLogEntry(id, time);
     this._mapTimeline.push(map);
     return map;
   }
 
-  getExistingMapEntry(id, time) {
+  getMapEntry(id, time) {
     if (id === '0x000000000000') return undefined;
-    let map = MapLogEntry.get(id, time);
-    if (map === undefined) {
-      console.error(`No map details provided: id=${id}`);
-      // Manually patch in a map to continue running.
-      return this.createMapEntry(id, time);
-    };
-    return map;
+    const map = MapLogEntry.get(id, time);
+    if (map !== undefined) return map;
+    console.warn(`No map details provided: id=${id}`);
+    // Manually patch in a map to continue running.
+    return this.createMapEntry(id, time);
   }
 
   getScript(url) {