Event Pipe File V3 #16107

adamsitnik · 2018-01-30T21:49:40Z

This PR is ready for review, but not ready to be merged yet.

Today all non-Windows build are going to fail because I have bumped the version of Event Pipe file, which is not supported by the referenced EventTrace library.

We need to release new version of TraceEvent lib and updated the references to it in CoreCLR, so all tests are going to pass. This + review and then it can be merge.

Changes:

New version of the Event Pipe file
Removed forward references (to avoid the need of reading entire file to read the value of forward reference to the end of event stream)
Introduced Event Blocks which are simply groups of events.
Ensured 4-byte alignment
Proper ending of the file and all objects (End tag was missing for EventPipe object)
Added missing data (pointer size, # of processors, process id, sampling rate)
Removed duplicated data (event id and event version_
Ensured that metadata payload is always present for the metadata events

I am a newbie when it comes to C++, please make sure that the review is very detailed.

/cc @jorive

…e it work fo x86)

…nsumers get clear error message

…the event block objects

This reverts commit 98ef2f5.

…ated with native code)

…, right after last events

…n, not manged

…need to serialize length)

# Conflicts: # src/vm/eventpipeeventinstance.h

@vancem

@vancem

brianrob

Thanks for making these changes. Looks good - some comments as well.

brianrob · 2018-02-01T16:22:35Z

src/vm/eventpipe.cpp

@@ -393,6 +393,8 @@ void EventPipe::Disable()
        s_pConfig->DeleteSession(s_pSession);
        s_pSession = NULL;

+        s_pFile->WriteEnd(); // write the rest of the events and end tag to the file


If possible, it would be nice to have this occur as part of the destructor of the EventPipeFile. This ensures that no matter how the file is used the end gets properly written. Note that there is at least one more EventPipeFile - the synchronously written file that would also need a call to WriteEnd to be functionally correct, though I'm not 100% sure that it still works today.

brianrob · 2018-02-01T16:24:31Z

src/vm/eventpipeblock.h

+
+        void FastSerialize(FastSerializer *pSerializer)
+        {
+            unsigned int eventsSize = (unsigned int)(m_pWritePointer - m_pBlock);


You should use size_t as the size here since it is guaranteed to allow you to represent all memory addresses (and sizes) for a given bitness machine.

brianrob · 2018-02-01T16:24:45Z

src/vm/eventpipeblock.h

+class EventPipeBlock : public FastSerializableObject
+{
+    public:
+        EventPipeBlock(unsigned int maxBlockSize);


size_t here as well.

same sizes of blocks can be 32 bit.

brianrob · 2018-02-01T16:30:11Z

src/vm/eventpipeblock.cpp

+    }
+    CONTRACTL_END;
+
+    m_pBlock = new BYTE[maxBlockSize];


As a philosophical statement, we say that we don't want failures in tracing to affect program correctness. Thus, one of the things we do is to try to make sure that we don't throw in places that will leak out to the program. The only exception that EventPipe has (or should have) currently is that we will throw if you try to enable tracing in-proc and it fails for some reason. Other than that, we'd rather lose data than affect program correctness.

Thus, here, you should use the nothrow flavor of new:

m_pBlock = new (nothrow) BYTE[maxBlockSize]; if(m_pBlock == NULL) { return; }

You will of course also need to handle a NULL m_pBlock inside the other functions of EventPipeBlock.

Nice! I had no idea about new (nothrow)

brianrob · 2018-02-01T16:32:27Z

src/vm/eventpipeblock.cpp

+        m_pWritePointer += stackSize;
+    }
+
+    while (m_pWritePointer < alignedEnd)


Nit: Please put { } around this loop.

brianrob · 2018-02-01T19:55:35Z

src/vm/eventpipefile.h

-        MapSHashWithRemove<EventPipeEvent*, StreamLabel> *m_pMetadataLabels;
+        MapSHashWithRemove<EventPipeEvent*, unsigned int> *m_pMetadataIds;
+
+        volatile unsigned int m_metadataIdCounter;


If you change the type to Volatile I don't think you'll need the casts when you call InterlockedIncrement.

brianrob · 2018-02-01T20:00:23Z

src/vm/eventpipefile.cpp

+
+    m_pBlock->Clear();
+
+    // "After the last EventBlock is emitted, the stream is ended by emitting a NullReference Tag which indicates that there are no more objects in the stream to read."


Accidental quotes on the comment?

It was a quote from the docs, I should have added link

brianrob · 2018-02-01T20:03:21Z

src/vm/eventpipefile.cpp

+
+    instance.SetMetadataId(metadataId);
+
+    if (m_pBlock->WriteEvent(instance))


brianrob · 2018-02-01T20:06:29Z

src/vm/eventpipefile.h

        {
-            LIMITED_METHOD_CONTRACT;
-            return "Microsoft.DotNet.Runtime.EventPipeFile";
+            pSerializer->WriteBuffer((BYTE*)&m_fileOpenSystemTime, sizeof(m_fileOpenSystemTime));


I like this! Much better that the file itself is represented in FastSerialization instead of being special cased.

brianrob · 2018-02-01T20:08:10Z

src/vm/eventpipefile.h

        }

-        // Get the type name of this object.
-        const char* GetTypeName()
+        void FastSerialize(FastSerializer *pSerializer)


I know that you got rid of the support for forward references as part of this change to make it easier to support streaming. How do you make sure that this block of information is extensible since the forward references are now gone?

See https://github.com/Microsoft/perfview/blob/master/src/TraceEvent/EventPipe/EventPipeFormat.md#versioning-the-format-while-maintaining-compatibility

brianrob · 2018-02-01T21:01:16Z

One other thing that we should look at is what the right default block size is so that we don't thrash too much but also don't allocate huge blocks.

vancem · 2018-02-01T21:55:33Z

I have updated http://nuget.org with version 2.0.4 of the TraceEvent library which has the EventPipe V3 format in it. @adamsitnik you can update your test code to point at it so it does not fail.

Note that I am assuming you know about the fact that you can create a Nuget.Config file at the base of your project and add additional locations to find nuget packages (see https://github.com/Microsoft/perfview/blob/master/Nuget.config) you can thus make your private version of the TraceEvent package and put it there and your test code will find it. This allows you to test everything without needing to update nuget.org.

One other thing that we should look at is what the right default block size is so that we don't thrash too much but also don't allocate huge blocks.

Note that my belief is that any number between 10K to 1Meg will work well enough for write perf. We currently use 100K and that is a fine number.

adamsitnik · 2018-02-02T13:30:55Z

One of the Ubuntu builds fails with 8:12: error: no matching function for call to 'InterlockedIncrement'

How can I increment a value in atomic, cross-platform way here? I understand that InterlockedIncrement that I have used is Windows-only?

I have ofc search for that on the web, but I can't see anybody using std::atomic in CoreCLR.

@brianrob @jorive any ideas?

brianrob · 2018-02-02T16:24:48Z

It looks like the PAL only implements InterlockedIncrement for LONG and LONGLONG (via InterlockedIncrement64). See https://github.com/dotnet/coreclr/blob/master/src/pal/inc/pal.h#L4159 for details. You just happen to be picking up the int version on Windows most likely.

Probably the best thing to do is to change the data type of the metadata ID counter to Volatile<LONG> and then cast the result to int.

vancem · 2018-02-02T16:36:43Z

@adamsitnik - Try FastInterlockIncrement. (although I must admit I am surprised that InterlockedIncrement did not work).

adamsitnik · 2018-02-02T20:25:27Z

@brianrob @vancem thanks for help and the review!

Brian, I just pushed last minor improvements after additional review from @jorive

Could you check the PR?

If you accept it, and all tests except the unstable HardwareInstrincts pass, who can merge it then? I have this right on GitHub but I don't know the rules.

brianrob

LGTM!

brianrob · 2018-02-02T20:38:40Z

@adamsitnik, once the CI runs complete and pass (or just have the hardware intrinsic failures) you can go ahead and merge. The default option (Squash and merge) is what you should use since you don't need to maintain distinct commits for this change. Requirements for merge are clean CI, MS employee review sign-off.

@vancem

* write missing information to the event pipe file (pointer size to make it work fo x86) * define where the events start, not only where they end * include process Id in the event pipe file, bump the version so old consumers get clear error message * write the missing EndObject tag to close the header * include expected CPU sampling rate in the event pipe header file * include keywords in V3 of EventPipe metadata, fixes dotnet/coreclr#11934 * remove forward references * entry object comes after the header and ends after it's data, before the event block objects * introduce event block * fix the GC contracts * generate metadata ids * end the file with null reference tag * getting it work * 4 byte alignment of serialized event data * Revert "include keywords in V3 of EventPipe metadata, fixes dotnet/coreclr#11934" This reverts commit dotnet/coreclr@98ef2f5. * remove event Id and event version from metadata buffer (it was duplicated with native code) * increase the block size to be the same as buffer size * Write the last event block to the file after disabling the event pipe, right after last events * include the sife in itself * the native part was supposed to not duplicate the event id and version, not manged * ensure 4 byte alignment * build metadata when it's not provided, so payload is never empty (no need to serialize length) * this todo is no longer valid * don't align everything, just the content of event block as suggested by @vancem * improvements after code review * update TraceEvent dependency, make the test verify new feature * InterlockedIncrement(Int32) is not available for non-Windows OSes * code improvements after Skype code review from @jorive Commit migrated from dotnet/coreclr@f6ba335

adamsitnik added 24 commits January 15, 2018 20:01

write missing information to the event pipe file (pointer size to mak…

893c86e

…e it work fo x86)

define where the events start, not only where they end

18dfeeb

include process Id in the event pipe file, bump the version so old co…

b018993

…nsumers get clear error message

write the missing EndObject tag to close the header

282e79a

include expected CPU sampling rate in the event pipe header file

2a0f168

include keywords in V3 of EventPipe metadata, fixes #11934

98ef2f5

remove forward references

839033d

entry object comes after the header and ends after it's data, before …

0629cf2

…the event block objects

introduce event block

93805cd

fix the GC contracts

cde0602

generate metadata ids

c2ce709

end the file with null reference tag

9e5d245

getting it work

071c407

4 byte alignment of serialized event data

b62f13c

Revert "include keywords in V3 of EventPipe metadata, fixes #11934"

138ed6a

This reverts commit 98ef2f5.

remove event Id and event version from metadata buffer (it was duplic…

abbc2d7

…ated with native code)

increase the block size to be the same as buffer size

09411ab

Write the last event block to the file after disabling the event pipe…

9871aa0

…, right after last events

include the sife in itself

a761aec

the native part was supposed to not duplicate the event id and versio…

63e9ca8

…n, not manged

ensure 4 byte alignment

323c3a0

build metadata when it's not provided, so payload is never empty (no …

4d9c9bd

…need to serialize length)

this todo is no longer valid

7d18fe3

Merge remote-tracking branch 'upstream/master' into eventPipesMoreInfo

2c2f453

# Conflicts: # src/vm/eventpipeeventinstance.h

adamsitnik added area-EventSource * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-Tracing labels Jan 30, 2018

adamsitnik requested review from brianrob and vancem January 30, 2018 21:49

adamsitnik mentioned this pull request Jan 30, 2018

Event blocks microsoft/perfview#546

Merged

adamsitnik added 2 commits February 1, 2018 14:09

don't align everything, just the content of event block as suggested by

0300fb5

@vancem

Merge remote-tracking branch 'upstream/master' into eventPipesMoreInfo

c137bc2

brianrob reviewed Feb 1, 2018

View reviewed changes

vancem approved these changes Feb 1, 2018

View reviewed changes

adamsitnik added 2 commits February 2, 2018 11:16

improvements after code review

ff9f616

update TraceEvent dependency, make the test verify new feature

afa9008

InterlockedIncrement(Int32) is not available for non-Windows OSes

122d19a

adamsitnik removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 2, 2018

code improvements after Skype code review from @jorive

e14496c

brianrob approved these changes Feb 2, 2018

View reviewed changes

adamsitnik merged commit f6ba335 into dotnet:master Feb 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event Pipe File V3 #16107

Event Pipe File V3 #16107

adamsitnik commented Jan 30, 2018

brianrob left a comment

brianrob Feb 1, 2018

brianrob Feb 1, 2018

brianrob Feb 1, 2018

vancem Feb 1, 2018

brianrob Feb 1, 2018

adamsitnik Feb 1, 2018

brianrob Feb 1, 2018

brianrob Feb 1, 2018

brianrob Feb 1, 2018

adamsitnik Feb 1, 2018

brianrob Feb 1, 2018

brianrob Feb 1, 2018

brianrob Feb 1, 2018

vancem Feb 1, 2018

brianrob commented Feb 1, 2018

vancem commented Feb 1, 2018

adamsitnik commented Feb 2, 2018

brianrob commented Feb 2, 2018 •

edited

Loading

vancem commented Feb 2, 2018

adamsitnik commented Feb 2, 2018

brianrob left a comment

brianrob commented Feb 2, 2018


		m_pBlock->Clear();

		// "After the last EventBlock is emitted, the stream is ended by emitting a NullReference Tag which indicates that there are no more objects in the stream to read."


		instance.SetMetadataId(metadataId);

		if (m_pBlock->WriteEvent(instance))

Event Pipe File V3 #16107

Event Pipe File V3 #16107

Conversation

adamsitnik commented Jan 30, 2018

brianrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob commented Feb 1, 2018

vancem commented Feb 1, 2018

adamsitnik commented Feb 2, 2018

brianrob commented Feb 2, 2018 • edited Loading

vancem commented Feb 2, 2018

adamsitnik commented Feb 2, 2018

brianrob left a comment

Choose a reason for hiding this comment

brianrob commented Feb 2, 2018

brianrob commented Feb 2, 2018 •

edited

Loading