[PATCH] D122156: [lld][Macho][NFC] Encapsulate priorities map in a priority class

[Roger Kim via Phabricator via llvm-commits]

Roger marked an inline comment as done.
Roger added inline comments.


================
Comment at: lld/MachO/SectionPriorities.cpp:305
   MemoryBufferRef mbref = *buffer;
-  size_t priority = std::numeric_limits<size_t>::max();
+  size_t priority = lowestPriority;
   for (StringRef line : args::getLines(mbref)) {
----------------
int3 wrote:
> Roger wrote:
> > int3 wrote:
> > > Roger wrote:
> > > > int3 wrote:
> > > > > Roger wrote:
> > > > > > int3 wrote:
> > > > > > > I think you're confused about the intention of the code. `numeric_limits::max()` is the *highest* priority, not the lowest. But it's used to initialize the default value of the `lowestPriority` variable because if there are no entries in the order file, then the lowest priority is also the highest priority.
> > > > > > > 
> > > > > > > Now that we have the `PriorityBuilder` class, I think `lowestPriority` can be a member there. And maybe add a comment so future readers won't get confused :)
> > > > > > `lowestPriority` actually represents the highest priority that has not been assigned (ie, the priority lower than the lowest priority that has so far been assigned). `lowestPriority` is used to keep track of the highest priority `CallGraphSort` should use because the code aims to make sure that all priorities assigned by `CallGraphSort` should be lower than those assigned by the order file.
> > > > > > 
> > > > > > At the time this line of code is called, `lowestPriority` will always be set to its initial value of `std::numeric_limits<size_t>::max()`. There is no need to specify `std::numeric_limits<size_t>::max()` both here and at the initialization of `lowestPriority`.
> > > > > > 
> > > > > > I considered making `lowestPriority` a member of PriorityBuilder but it would be a bit more than just moving the field and changing some names because `lowestPriority` is read and written to by `CallGraphSort` as well. I didn't include that change here but it could possibly be done.
> > > > > ah okay, I see how you're thinking about it. I still think `priority = lowestPriority` is confusing though, because it's not obvious that we really mean "lowest priority unassigned by the order file" instead of "lowest possible priority". Also come to think of it, the local `priority` variable is kind of redundant. How about using `lowestPriority` directly but renaming it to something like `lowestUnusedPriority`?
> > > > Actually, it may be better to rename `lowestPriority` to `highestAvailablePriority`.
> > > sgtm
> > As I was reading the code, I got the sense that assigning `priority = lowestPriority` was done intentionally. I was wondering if this is a speed optimization (incrementing the stack copy `priority` many times and writing to the global `lowestPriority` just at the end). WDYT?
> > As I was reading the code, I got the sense that assigning priority = lowestPriority was done intentionally. I was wondering if this is a speed optimization (incrementing the stack copy priority many times and writing to the global lowestPriority just at the end). WDYT?
> 
> I don't think you posted this comment before I replied  (my "sgtm" was in response to the rename), but it seems like Phab has recorded them out of order...
> 
> I doubt it's an intentional speed optimization; the difference is really not that significant, and I would expect a comment or something if profiling had indeed identified this as a bottleneck. Otherwise it's just unnecessary microoptimization :)
> 
> Also given that it's a static, there's a decent chance the compiler can optimize it anyway.
Gotcha. I removed the `priority` variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122156/new/

https://reviews.llvm.org/D122156