Hello Gem5 Developers,

I want to redefine PacketPtr from Packet * to shared_ptr<Packet>, and make all the needed associated changes.  This is to remove to need to explicitly delete a Packet *, reducing the risk of making a memory leak or corruption error.  I think this change is going to make gem5 more stable, but is going to be a big change as Packet is used in many places.

I would like to get some feedback from the community regarding whether this is going to be an acceptable change.  Is it too big and risky for the benefits?  I've made the change in my local checkout and about 193 files were touched.

Thank you.

Hi Anhdung,

I would really welcome this change. There is something you should be aware though:

I tried to do the same few years ago (I aliased as you did the PacketPtr with shared_ptr) and what I noticed was a sensible slowdown in simulation (due to the ref-counting). I don’t remember the exact amount, but it was something like in the 20% ballpark.

So the caveat is that you should check for performance regressions as we should make sure we are not hurting performance that much. I might have been doing something wrong last time so you might get better results.

In case you also see a sensible slowdown, here’s what I would recommend you to try:

1. It’s obvious but try to pass the PacketPtr as const ref as much as you can
2. Try to use RefCountingPtr 1 which is not using “expensive” atomics and see if you get better results
3. We probably store a copy of the PacketPtr somewhere in the memsys. Is it always necessary?

Kind Regards

Giacomo

From: Anhdung Duong Ngo via gem5-dev gem5-dev@gem5.org
Date: Thursday, 24 October 2024 at 22:30
To: The gem5 Developer List gem5-dev@gem5.org
Cc: Anhdung Duong Ngo anh.ngo1@samsung.com
Subject: [gem5-dev] Redefining PacketPtr to shared_ptr<Packet>
Hello Gem5 Developers,

I want to redefine PacketPtr from Packet * to shared_ptr<Packet>, and make all the needed associated changes.  This is to remove to need to explicitly delete a Packet *, reducing the risk of making a memory leak or corruption error.  I think this change is going to make gem5 more stable, but is going to be a big change as Packet is used in many places.

I would like to get some feedback from the community regarding whether this is going to be an acceptable change.  Is it too big and risky for the benefits?  I’ve made the change in my local checkout and about 193 files were touched.

Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Hello Giacomo,

Thank you for your feedback.
I think suggestion 1 is the most practical and will try to do that.  It alone can substantially reduce the performance penalty.

A couple of questions I have:

1. What is (simulation) performance regression that you mentioned.  I am going to use LmBench and SpecInt to measure simulation performance.  Is that good enough?
2. I am planning on using simulation runtime as the simulation performance metric.  Is that good?  My company compute farm has job start-time and end-time, but that can metric can vary depending on the machine the job lands.

From: Giacomo Travaglini via gem5-dev gem5-dev@gem5.org
Sent: Friday, October 25, 2024 3:47 AM
To: The gem5 Developer List gem5-dev@gem5.org
Cc: Anhdung Duong Ngo anh.ngo1@samsung.com; Giacomo Travaglini Giacomo.Travaglini@arm.com
Subject: [gem5-dev] Re: Redefining PacketPtr to shared_ptr<Packet>

Hi Anhdung,

I would really welcome this change. There is something you should be aware though:

I tried to do the same few years ago (I aliased as you did the PacketPtr with shared_ptr) and what I noticed was a sensible slowdown in simulation (due to the ref-counting). I don't remember the exact amount, but it was something like in the 20% ballpark.

So the caveat is that you should check for performance regressions as we should make sure we are not hurting performance that much. I might have been doing something wrong last time so you might get better results.

In case you also see a sensible slowdown, here's what I would recommend you to try:

1. It's obvious but try to pass the PacketPtr as const ref as much as you can
2. Try to use RefCountingPtr 1 which is not using "expensive" atomics and see if you get better results
3. We probably store a copy of the PacketPtr somewhere in the memsys. Is it always necessary?

Kind Regards

Giacomo

From: Anhdung Duong Ngo via gem5-dev <gem5-dev@gem5.orgmailto:gem5-dev@gem5.org>
Date: Thursday, 24 October 2024 at 22:30
To: The gem5 Developer List <gem5-dev@gem5.orgmailto:gem5-dev@gem5.org>
Cc: Anhdung Duong Ngo <anh.ngo1@samsung.commailto:anh.ngo1@samsung.com>
Subject: [gem5-dev] Redefining PacketPtr to shared_ptr<Packet>
Hello Gem5 Developers,

I want to redefine PacketPtr from Packet * to shared_ptr<Packet>, and make all the needed associated changes.  This is to remove to need to explicitly delete a Packet *, reducing the risk of making a memory leak or corruption error.  I think this change is going to make gem5 more stable, but is going to be a big change as Packet is used in many places.

I would like to get some feedback from the community regarding whether this is going to be an acceptable change.  Is it too big and risky for the benefits?  I've made the change in my local checkout and about 193 files were touched.

Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.