New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 597828 link

Starred by 1 user
Status: Archived
Owner:
█ chongjiang@chromium.org
Last visit > 30 days ago
Closed: Jan 2017
Cc:
█ dhsharp@chromium.org
sque@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
cwp



Sign in to add a comment

Split up quipper's utils module

Project Member Reported by sque@chromium.org, Mar 24 2016 
This is just a catch-all for various standalone functions. We should split it up by function category.

The motivation is to avoid circular dependencies between Perf{Reader|Parser|Serializer}. Currently TrimZeroesFromBuildIDString is defined in Reader and used in Serializer.

I propose:

perf_data_utils:
- malloced_unique_ptr
- CallocMemoryForEvent
- ReallocMemoryForEvent
- CallocMemoryForBuildID
- GetSampleInfoForEvent
- GetTimeFromPerfEvent
- PerfizeBuildIDString (from perf_reader)
- TrimZeroesFromBuildIDString (from perf_reader)

raw_data_utils:
- ByteSwap
- MaybeSwap
- Md5Prefix (x 2)
- RawDataToHexString (x 2)
- HexStringToRawData
- GetNumBits
- Align
- GetUint64AlignedStringLength

file_utils:
- GetFileSizeFromHandle
- FileToBuffer
- BufferToFile
- FileExists
- ReadFileToData (redundant?)
- WriteFileToData (redundant?)

string_utils:
- TrimWhiteSpace
- SplitString

Comment 1 by dhsharp@chromium.org, Mar 25 2016 

GetUint64AlignedStringLength probably belongs in perf_data_utils. Otherwise looks like a good division.

How about "binary(_data)?_utils" rather than "raw_data". I think "binary" describes what those functions deal with more aptly. "raw" really means something closer to "original", and a human-readable string can be just as original as binary data.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c0c58a464af643d505e269064361713dad06f4a6

commit c0c58a464af643d505e269064361713dad06f4a6
Author: Simon Que <sque@chromium.org>
Date: Thu Mar 24 22:58:51 2016

quipper: Create perf_data_utils module out of utils

BUG= chromium:597828 
TEST=unit tests pass

Change-Id: I968378d15e237e7be866feac255f789a25572ae9
Reviewed-on: https://chromium-review.googlesource.com/334871
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Simon Que <sque@chromium.org>
Reviewed-by: David Sharp <dhsharp@chromium.org>

[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_serializer.h
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/data_writer.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/utils.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_serializer.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/utils.h
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/Makefile.external
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/utils_test.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_reader_test.cc
[add] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_data_utils.h
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/test_perf_data.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/quipper.gyp
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_serializer_test.cc
[add] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_data_utils_test.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/sample_info_reader.cc
[add] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_data_utils.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_reader.cc
[modify] https://crrev.com/c0c58a464af643d505e269064361713dad06f4a6/chromiumos-wide-profiling/perf_reader.h

Comment 3 by sque@chromium.org, Dec 20 2016

Cc: sque@chromium.org
Owner: chongjiang@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/11a91d4851c0a2a4e18490627ca5b01808ea50ca

commit 11a91d4851c0a2a4e18490627ca5b01808ea50ca
Author: Chong Jiang <chongjiang@chromium.org>
Date: Fri Dec 23 01:22:43 2016

quipper: split string_utils out of utils

BUG= chromium:597828 
TEST=unit tests pass

Change-Id: If196db9e6540eddd6e2f76d1c088b556a5dbe7e7
Reviewed-on: https://chromium-review.googlesource.com/423260
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>

[add] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/string_utils.cc
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/utils.cc
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/utils.h
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/Makefile.external
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/perf_stat_parser.cc
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/test_utils.cc
[modify] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/quipper.gyp
[add] https://crrev.com/11a91d4851c0a2a4e18490627ca5b01808ea50ca/chromiumos-wide-profiling/string_utils.h
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/39e974ac69c34f54028ebb17e6d75eb56858dc50

commit 39e974ac69c34f54028ebb17e6d75eb56858dc50
Author: Chong Jiang <chongjiang@chromium.org>
Date: Fri Dec 23 02:05:27 2016

quipper: split file_utils out of utils

Also remove unused and duplicate functions.

BUG= chromium:597828 
TEST=unit tests pass

Change-Id: Ibde03e26f4846695ff2c5d72f0807bdef5f15587
Reviewed-on: https://chromium-review.googlesource.com/423261
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>

[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/conversion_utils.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/quipper.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/utils.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/utils.h
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/Makefile.external
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_stat_parser.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_reader.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/test_utils.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/file_reader_test.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/quipper.gyp
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_serializer_test.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_reader_test.cc
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_stat_parser_test.cc
[add] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/file_utils.cc
[add] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/file_utils.h
[modify] https://crrev.com/39e974ac69c34f54028ebb17e6d75eb56858dc50/chromiumos-wide-profiling/perf_protobuf_io.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795

commit 4dd7a7c92b7e729f28e8569e5bc0b6f888a11795
Author: Chong Jiang <chongjiang@chromium.org>
Date: Fri Dec 23 02:25:32 2016

quipper: rename utils to binary_data_utils

BUG= chromium:597828 
TEST=unit tests pass

Change-Id: I91c5e99a47641d61e0306d2785c8333917f8bb59
Reviewed-on: https://chromium-review.googlesource.com/423262
Commit-Ready: Simon Que <sque@chromium.org>
Tested-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>

[rename] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/binary_data_utils_test.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_parser.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_serializer.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/dso_test_utils.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/Makefile.external
[rename] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/binary_data_utils.cc
[rename] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/binary_data_utils.h
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_recorder.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_data_utils.h
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/test_perf_data.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/data_reader.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_parser.h
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/quipper.gyp
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/data_reader.h
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_reader.cc
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/perf_reader.h
[modify] https://crrev.com/4dd7a7c92b7e729f28e8569e5bc0b6f888a11795/chromiumos-wide-profiling/test_perf_data.h
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 2 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/f48e135cdc6b5aa37ebed12b2e7611abe8412964

commit f48e135cdc6b5aa37ebed12b2e7611abe8412964
Author: Chong Jiang <chongjiang@chromium.org>
Date: Tue Dec 27 21:04:41 2016

quipper: remove unused file_reader dependency

BUG= chromium:597828 
TEST=unit tests pass

Change-Id: I6c342c211ef20634ae581bba470f6e5250f1f91f
Reviewed-on: https://chromium-review.googlesource.com/424107
Commit-Ready: Chong Jiang <chongjiang@chromium.org>
Tested-by: Chong Jiang <chongjiang@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>

[modify] https://crrev.com/f48e135cdc6b5aa37ebed12b2e7611abe8412964/chromiumos-wide-profiling/binary_data_utils.cc

Comment 10 by chongjiang@chromium.org, Jan 5 2017

Status: Fixed (was: Assigned)

Comment 11 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 12 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 13 by dchan@google.com, May 30 2017

Labels: VerifyIn-60

Comment 14 by dchan@chromium.org, Aug 1 2017

Labels: VerifyIn-61

Comment 15 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)
►

Sign in to add a comment