Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3546: don't die if sysconf() reports bogus cache info
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3110/1/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:

Line 146:                << cache_line_sizes_[0] << "-byte L1 cache lines";
> is this going to be more confusing than it's worth? in reality, the cache l
Removed.


Line 192:   string L1 = Substitute("L1 Cache: $0 (Line: $1)",
        :       PrettyPrinter::Print(CacheSize(L1_CACHE), TUnit::BYTES),
        :       PrettyPrinter::Print(CacheLineSize(L1_CACHE), TUnit::BYTES));
        :   string L2 = Substitute("L1 Cache: $0 (Line: $1)",
        :       PrettyPrinter::Print(CacheSize(L2_CACHE), TUnit::BYTES),
        :       PrettyPrinter::Print(CacheLineSize(L2_CACHE), TUnit::BYTES));
        :   string L3 = Substitute("L1 Cache: $0 (Line: $1)",
        :       PrettyPrinter::Print(CacheSize(L3_CACHE), TUnit::BYTES),
        :       PrettyPrinter::Print(CacheLineSize(L3_CACHE), TUnit::BYTES));
> Since the cache info is never used anywhere else (and I guess we can't rely
I removed the members and just have a helper function to get the cache info. I 
think the logic is best to keep in a separate function since we have the Mac OS 
X-specific stuff too.


-- 
To view, visit http://gerrit.cloudera.org:8080/3110
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4d61b05fe213028a7e9aaabe98adc2792b90e07
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Casey Ching <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-HasComments: Yes

Reply via email to