Any views on this, otherwise I might just make an executive decision...? (probably my decision will be to add a 64-bit type)

Some of them can obviously be downcast to 32-bit but others are less obvious such as 

../../base/task/sequence_manager/task_queue_impl.cc:529:40: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'int' [-Wshorten-64-to-32]
    state->SetInteger("current_fence", main_thread_only().current_fence);
    ~~~~~                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

current_fence is explicitly a 64-bit integer here.

adding more people to try and elicit a response.

you can see the full list of warnings here:

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8935654763159086400/+/steps/compile__with_patch_/0/stdout

search for "SetInteger".

The base::trace_event::TracedValue is used to efficiently encode the data into a temporary buffer, so we can later read it back out and construct JSON (see https://cs.chromium.org/chromium/src/base/trace_event/trace_event_argument.cc?l=525).

The members used for integers in base::trace_event::TraceEvent, to pass to AppendValueAsJSON(), are 64-bit. So if we added a 64-bit type to TracedEvent then we could retain the full values, in principle.

primiano: Are these code paths going to go away when we switch to the new tracing impl?

fdoray: How quickly can |current_fence| wrap 32-bits in practice?  And do we need the base::Time internal values in the TestPendingTask impl, or could we instead use InSecondsF() to get the values as doubles, for testing, for example?

It's highly unlikely that |current_fence| is going to get bigger than 2^31 in practice (we'll need to 

run 2^31 tasks in one process. That can happen for very long-living (weeks or months) processes, but for tracing that's fine to truncate the value.

I don't think we should be truncating if we don't have to. If the values are never > 32-bit then they should just be 32-bit types. Then the classes that use these types can check for overflows within themselves (e.g. when incrementing, or setting).

so, e.g. current_fence, *._content_area, and all other types that are 64-bit should be 32-bit? The only ones that are really 64-bit seem to be the timestamp ones, and that's because the base API returns 64-bit values back.

Either we need to add support for 64-bit types as wez says in #4, or we need to change these to 32-bit.

|current_fence| should be 64-bit type because scheduling infrastructure relies on each task having an increasing unique id. Renderer process would stop working otherwise, so it's important to protect about these rare scenarios, but for tracing it's not so critical. Having int64_t support in tracing is nice, but not that important.

IIUC the argument altimin@ made in #5 / #6 was that for tracing purposes the range of values in the 64-bit counters won't exceed 32-bit, since you never trace a process for ~weeks.  OTOH, people do leave the browser running for ~weeks, so we probably can't safely make the fence counter 32-bit. Extending the tracing internal buffer format to store 64-bit values will increase the size of the traces, with no actual benefit (if we assume a trace never overflows 32-bit).

One way to balance the two would be to write a zero-based 32-bit counter value, relative to the value when tracing was started; we'd then use a checked-cast to protect against overflow while actually tracing. Not sure that feasibility/complexity/value works out for that, though.

sorry for the delay... perf.
So the problem is that until we have to support the JSON output (for a while for things like devtools) we can't reliably add int64, because we'd be really able to support 2^^53. It's not a JSON limitation itself, but more the fact that JS turns that into a IEEE754 double.
So int64, as is, could end up creating a lot of surprise factors.
I think for now we should just add casts to the call sites .

If we simply add casts then there's nothing stopping us happening to take a
trace across a 31-bit overflow - is that something we can cope with...?

I really don't like adding casts unless it's absolutely necessary, especially when we "expect" them to wrap. I suppose it might be acceptable to "& 0xFFFFFFFFU" and make the wrapping explicit... but...

if MainThreadOnly::current_fence needs to be 64-bit legitimately then it seems to be there should be an API perhaps exposed at EnqueueOrder for getting a 32-bit relative trace value, after all the meaning of current_fence seems internal to EnqueueOrder and yet it's being pulled directly out for tracing... something that is guaranteed to increase but might wrap (and any consumers of the value must be aware it will wrap, and we should add tests for this).

e.g. someone starts a trace right before the 32-bit wrapping is about to happen, it sounds like we need to handle that gracefully.

Perhaps the real solution here is to use a relative counter and convert to 32-bit (comment #9).

Now I'm just rambling. But I'm uncomfortable with casts here.

re: #10 we could add int64 type but just CHECK() if the value ever exceeds 2^^53 :)

https://www.google.com/search?q=2%5E53+microseconds

"New bug report: I ran Chrome for 285 years and it crashed!"

I don't think wrapping itself is a problem. The problem is not getting colliding numbers within a trace, which should be rare even when wrapping, I think (or maybe I am not thinking to something. Does anything rely on relative ordering here?). I mean we are already in this state today, we just never realized until now, no?

> re: #10 we could add int64 type but just CHECK() if the value ever exceeds 2^^53 :)

My worry with that is that somebody sees int64_t and casts a pointer into that.
Those numbers are not just used for counters (in which case I agree with you, 285 y are enough). They can be used as identifiers / references. As a user I'd be quite surprised if a method that takes a int64_t crashes after 2**53.
And this is what I never liked floating point numbers.

I just want to add that even if we come up with a solution for |current_fence| there's a load of others. Ones with * look a lot like |current_fence|. Ones with + seem to be checked_cast-able. The ToInternalValues look sketchy: 32-bit microseconds overflows in just over an hour.

base/task/sequence_manager/task_queue_impl.cc:
  state->SetInteger("current_fence", main_thread_only().current_fence); *
  state->SetInteger("enqueue_order", task.enqueue_order()); *

components/viz/common/frame_sinks/begin_frame_args.cc:
  state->SetInteger("source_id", source_id); *
  state->SetInteger("sequence_number", sequence_number); *

components/viz/common/frame_sinks/begin_frame_source.cc:
  state->SetInteger("dropped_begin_frame_args", dropped_begin_frame_args_); +
  state->SetInteger("num_observers", observers_.size()); +

state->SetInteger("worst_sample_count", worst_sample_count); +(maybe)

base/test/test_pending_task.cc
  state->SetInteger("run_at", GetTimeToRun().ToInternalValue());
  state->SetInteger("post_time", post_time.ToInternalValue());
  state->SetInteger("delay", delay.ToInternalValue());
  state->SetInteger("delay", delay.ToInternalValue());

Would it make sense to switch the time values to Time::InSecondsF() doubles?