-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change for split requests #257
base: jason/dynamorio-traces
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that's missing is updates to numOutstandingMemReqs
. Right now, this is going to go below 0 (we should probably add an assert on (new) line 320.
Right now, there is just one memory request added for a split packet, but we are subtracting twice. We should either only subtract when we receive both packets or we should add twice. I think the latter would be better.
@@ -230,9 +261,35 @@ DRTracePlayer::getPacket(DRTraceReader::TraceRef &mem_ref) | |||
} | |||
} | |||
|
|||
return pkt; | |||
} | |||
PacketPtr split_pkt = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PacketPtr split_pkt = NULL; | |
PacketPtr split_pkt = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PacketPtr pkt, split_pkt; | ||
|
||
std::tie(pkt, split_pkt) = getPacket(mem_ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PacketPtr pkt, split_pkt; | |
std::tie(pkt, split_pkt) = getPacket(mem_ref); | |
auto [pkt, split_pkt] = getPacket(mem_ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
} | ||
|
||
return std::make_pair(pkt, split_pkt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following works in modern C++.
return std::make_pair(pkt, split_pkt); | |
return {pkt, split_pkt}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return false; | ||
} | ||
} | ||
|
||
PacketPtr | ||
std::pair<PacketPtr, PacketPtr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is right, but you should double check me :).
std::pair<PacketPtr, PacketPtr> | |
std::tuple<PacketPtr, PacketPtr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (stats.memStallStart == 0) { | ||
stats.memStallStart = curTick(); | ||
} | ||
numOutstandingMemReqs++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we increment the outstanding requests when the packet is failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I pushed the wrong version earlier
if (stats.memStallStart == 0) { | ||
stats.memStallStart = curTick(); | ||
} | ||
delete pkt; | ||
numOutstandingMemReqs++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why increcement when it fails?
// we should not be sending the first pkt again if | ||
// we are retrying only on the second part of the | ||
// previously stalled request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is for the other side of the branch, not this side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have understood correctly, the place of the comment should be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added/fixed comments to hopefully make things better and not worse
// Also, the assumption is that we cannot start a | ||
// new memory request in parallel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to interpret this statement. Otherwise, it looks good.
Has not been extensively tested yet.