621084 - Constify/privatize GCAlloc

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Simon Wilkinson]

Attachment: Latest patch. MQ rev 162

Constifies and privatizes GCAlloc state.

This is preparatory patch for asymmGC (bug 582770)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 499448 [details] [diff] [review]
Latest patch. MQ rev 162

Note: you've turned one invocation of ComputeMultiplyShift into two.  Its loop is bounded time, but I still bristle a bit at it.

The only other option I can think of is pretty ugly though: use a single invocation that, yuck, returns multiple by value and sets a temporary t_shift by reference, and t_shift in turn is declared as a default-value parameter in the GCAlloc ctor, [double-yuck]:


  struct A {
      A(int ty=0);
      int const m_x;
      int const m_y;
      /* returns x, sets y. */
      static int compute(int &y);
  };

  A::A(int ty) : m_x(compute(ty)), m_y(ty) { };

R+ for the patch as is; I'll leave the question of whether its double-invocation is acceptable compared to the alternative in your (and Lars') hands.

Comment 2

[Simon Wilkinson]

(In reply to comment #1)
> Comment on attachment 499448 [details] [diff] [review]
> Latest patch. MQ rev 162
> 
> Note: you've turned one invocation of ComputeMultiplyShift into two.  Its loop
> is bounded time, but I still bristle a bit at it.

> R+ for the patch as is; I'll leave the question of whether its
> double-invocation is acceptable compared to the alternative in your (and Lars')
> hands.

I know it's not the most elegant solution, but it's not exactly on a hot path.

Comment 3

[Lars T Hansen]

Comment on attachment 499448 [details] [diff] [review]
Latest patch. MQ rev 162

Passing a flag to "ComputeMultiplyOrShift" is rather less elegant than having two inline functions, one called ComputeMultiply and the other ComputeShift, but it hardly matters much.  Efficiency-wise I don't care, the code will never be hot.  (If we cared we would have a precomputed table anyway?)

Comment 4

[Steven Johnson]

(In reply to comment #3)
> Passing a flag to "ComputeMultiplyOrShift" is rather less elegant than having
> two inline functions, one called ComputeMultiply and the other ComputeShift,
> but it hardly matters much.  Efficiency-wise I don't care, the code will never
> be hot.  (If we cared we would have a precomputed table anyway?)

No, but stylistically, functions that have arguments that are *always* constant at compiletime are IMHO suboptimal style (unless it's due to, e.g., template-fu hackery); at the point of reading you must parse "true" as "do this specific thing" -- breaking into separate names would be more readable and thus preferable.

Comment 5

[Simon Wilkinson]

Attachment: Latest patch. MQ rev 175

Post-review change:

ComputeMultiplyOrShift is split into ComputeMultiply and ComputeShift

Comment 6

[Simon Wilkinson]

Felix, this is ready to land if everyone is happy with ComputeMultiply and ComputeShift. Thanks.

Comment 7

[Tamarin Bot]

changeset: 5719:7db74cb7df9d
user:      Simon Wilkinson <[e-mail]>
summary:   Bug 621084 - Constify/privatize GCAlloc (r=fklockii, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/7db74cb7df9d