912496 - Store source file names only once in tracelogging.log and refer to them by auto-incrementing id after that.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Till Schneidereit [:till]]

Attachment: Store source file names only once in tracelogging.log and refer to them by auto-incrementing id after that.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 799488 [details] [diff] [review]
Store source file names only once in tracelogging.log and refer to them by auto-incrementing id after that.

Review of attachment 799488 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Can I make an additional cosmetic request. Could you change 
void TraceLogging::log(Type type, const char* file, unsigned int lineno); to
void TraceLogging::log(Type type, const char* text = NULL, unsigned int number = 0)

And change sourceMap to textMap, sourceId to textId. And also the entry names.

::: js/src/TraceLogging.cpp
@@ +90,5 @@
>  
>  TraceLogging::TraceLogging()
> +  : startupTime(rdtsc()),
> +    loggingTime(0),
> +    sourceMap(std::map<const char *, uint32_t>()),

Since we don't care about order. Please use unordered map like hashmap. That's faster. I'm not sure what we use in firefox for this.

@@ +91,5 @@
>  TraceLogging::TraceLogging()
> +  : startupTime(rdtsc()),
> +    loggingTime(0),
> +    sourceMap(std::map<const char *, uint32_t>()),
> +    nextSourceId(0),

nextSourceId(1), and use 0 when there is no text

@@ +150,5 @@
> +        if (sourceMap.find(file) == sourceMap.end()) {
> +            // Copy the source file name,
> +            // because original could already be freed before writing the log file.
> +            sourceFile = strdup(file);
> +            sourceMap[file] = nextSourceId++;

Could this be fallible? Please add checks for OOM situations.

::: js/src/TraceLogging.h
@@ +47,5 @@
>  
>    private:
>      struct Entry {
>          uint64_t tick_;
> +        bool includeSource_;

Remove this boolean and use sourceId_ == 0 to know if there is information

@@ +52,1 @@
>          char* file_;

This can get removed too? We can get the name back by doing sourceMap[sourceId_]

Comment 3

[Till Schneidereit [:till]]

Attachment: v2

I addressed all feedback except for:

> Could this be fallible? Please add checks for OOM situations.

It could, but I'm not sure how we should deal with it. The strdup that was already there was fallible, too, and went unhandled. Should we just set a flag and stop logging altogether? Or drop this one entry? The latter would be difficult, because we'd then later have an unmatched stop event in the log.

> This can get removed too? We can get the name back by doing
> sourceMap[sourceId_]

We can't because then we don't know which entry was supposed to introduce the string into the log file. But maybe I'm missing something.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 799789 [details] [diff] [review]
v2

Review of attachment 799789 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Till Schneidereit [:till] from comment #3)
> > This can get removed too? We can get the name back by doing
> > sourceMap[sourceId_]
> 
> We can't because then we don't know which entry was supposed to introduce
> the string into the log file. But maybe I'm missing something.

I think we don't need to output the string in between the logs.
Before logging everything, we can output a textmap linking the numbers with the output.
(Can you open a follow-up bug for this?)

::: js/src/TraceLogging.cpp
@@ +149,5 @@
> +    if (text) {
> +        TextHashMap::AddPtr p = textMap.lookupForAdd(text);
> +        if (!p) {
> +            // Copy the source file name,
> +            // because original could already be freed before writing the log file.

Adjust comment to use "text" instead of "file"

@@ +152,5 @@
> +            // Copy the source file name,
> +            // because original could already be freed before writing the log file.
> +            text_ = strdup(text);
> +            textId = nextTextId++;
> +            textMap.add(p, text, textId);

Good call. I indeed forget to check the result of strdup. Can you add check for strdup and textMap failures and just return for now?

Comment 5

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba28223861b1

I agree on the text map, filing a bug now.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba28223861b1