Force cr_ prefix in log tags |
||
Issue descriptionFor every log message, we do what appears to me an unnecessary amount of processing for each tag. https://chromium.googlesource.com/chromium/src/+/f58f2b25669173bf3032225603725a2469e01ff1/base/android/java/src/org/chromium/base/Log.java#65 public static String normalizeTag(String tag) { if (tag.startsWith(sTagPrefix)) return tag; int unprefixedTagStart = 0; if (tag.startsWith(sDeprecatedTagPrefix)) { unprefixedTagStart = sDeprecatedTagPrefix.length(); } return sTagPrefix + tag.substring(unprefixedTagStart, tag.length()); } Is this harmless? Too much pre-optimization on my part? We are doing at a minimum a string compare for each log message. In the event it is wrong (cr.) or unprefixed, we then are building a new string each time (hopefully a no-op due to how string uses an internal pool). My thinking is that we should migrate all tags to have the cr_ prefix, then change the normalizeTag to just dcheck that it starts with cr_ to avoid slowing down any prod code. Maybe the right step is to do some profiling to see if it actually matters or not?
,
Jul 18 2017
I think the reasoning was that most logs are debug and are removed from the release builds. The other logs are about errors or some similar exceptional case where we commit to perform an I/O operation to write about it, so the cost of this additional processing should be minimal in comparison. Also, this was written when we had no reliable way to do a DCHECK in java, as asserts were always disabled in ART devices. That being said, we probably should remove the usages of "cr." (I got 30 hits on codesearch) and assert for it instead of switching it in normalizeTag. I do like the automatic prefixing with cr_, as it ensures consistency without the authors having to do anything, so I would prefer if we kept it. The best would be if we didn't have to care about the tags at all...
,
Jul 18 2017
Ah ha...I didn't see the @RemovableInRelease on most of the log entry points. That likely mitigates all of my performance concern.
I still think it is unfortunate that we have some code that adds cr_ and some that doesn't (excluding the legacy cr.). I think that requires "a bit" too much thinking when it comes to writing a tag and doing code reviews. I think it would be better to either force all tags be prefixed with cr_ or force them to not be prefixed with anything and then automatically add cr_ always.
i.e. normalizeTag would become:
assert !tag.startsWith("cr.");
assert !tag.startsWith("cr_");
return "cr_" + tag;
I think supporting both seems unnecessarily flexible. None the less, the fact that we are not wasting release build time on this makes this likely not worth the investment (especially if we don't all agree). I'll close this out for now. Thanks for the discussion!
|
||
►
Sign in to add a comment |
||
Comment 1 by tedc...@chromium.org
, Jul 17 2017