Closed Bug 912496 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: till, Assigned: till)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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_]
Attachment #799488 - Flags: review?(hv1989)
Attached patch v2Splinter Review
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.
Attachment #799789 - Flags: review?(hv1989)
Attachment #799488 - Attachment is obsolete: true
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?
Attachment #799789 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba28223861b1

I agree on the text map, filing a bug now.
Blocks: 912942
https://hg.mozilla.org/mozilla-central/rev/ba28223861b1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.