Re: [PATCH 8/9] d3drm: Add refcount info to AddRed and Release traces.
Le 09/03/2012 13:49, Nikolay Sivov a écrit :
On 3/9/2012 14:46, Christian Costa wrote:
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c
index 879d8b0..42d722c 100644
--- a/dlls/d3drm/d3drm.c
+++ b/dlls/d3drm/d3drm.c
@@ -87,10 +87,11 @@ static HRESULT WINAPI
IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID
static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface)
{
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI
IDirect3DRMImpl_Release(IDirect3DRM* iface)
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
ULONG ref = InterlockedDecrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref)
HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c
index 559128e..6f9802a 100644
--- a/dlls/d3drm/device.c
+++ b/dlls/d3drm/device.c
@@ -93,10 +93,11 @@ static HRESULT WINAPI
IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2*
static ULONG WINAPI
IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface)
{
IDirect3DRMDeviceImpl *This =
impl_from_IDirect3DRMDevice2(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p)\n", This);
+TRACE("(%p): AddRef from %d\n", This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
I personally don't see a reason to trace actual reference on release
and previous one on AddRef. I usually trace new value in both calls
so you don't need to keep in mind this +/- 1 offset when looking at
logs.
Also you don't need extra words in trace message cause function name
is visible in traces already.
Indeed that does not make sense but I've a always seen that. In
addition I took the message from another place.
I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I
will stick to it.
This is too verbose too, format like (%p)->(%d) is enough. You don't
usually look at refcount traces while debugging, but when you see it
traced value should be consistent so you don't need to think is it
'new ref' or 'old ref'.
->(...) are for arguments and I would keep 'new' to avoid confusion.
Maybe I could remove 'ref' but hey I don't think it's so verbose, it
take one line anyway.
I never have problem with refcount traces. This is the first thing I
look at when I debug a crash. Using a dedicated channel for refcount
would be better to reduce verbosity IMO.
Re: [PATCH 8/9] d3drm: Add refcount info to AddRed and Release traces.
On 3/9/2012 14:46, Christian Costa wrote:
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c
index 879d8b0..42d722c 100644
--- a/dlls/d3drm/d3drm.c
+++ b/dlls/d3drm/d3drm.c
@@ -87,10 +87,11 @@ static HRESULT WINAPI
IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID
static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface)
{
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI
IDirect3DRMImpl_Release(IDirect3DRM* iface)
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
ULONG ref = InterlockedDecrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref)
HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c
index 559128e..6f9802a 100644
--- a/dlls/d3drm/device.c
+++ b/dlls/d3drm/device.c
@@ -93,10 +93,11 @@ static HRESULT WINAPI
IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2*
static ULONG WINAPI
IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface)
{
IDirect3DRMDeviceImpl *This =
impl_from_IDirect3DRMDevice2(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p)\n", This);
+TRACE("(%p): AddRef from %d\n", This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
I personally don't see a reason to trace actual reference on release
and previous one on AddRef. I usually trace new value in both calls
so you don't need to keep in mind this +/- 1 offset when looking at
logs.
Also you don't need extra words in trace message cause function name
is visible in traces already.
Indeed that does not make sense but I've a always seen that. In
addition I took the message from another place.
I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I
will stick to it.
This is too verbose too, format like (%p)->(%d) is enough. You don't
usually look at refcount traces while debugging, but when you see it
traced value should be consistent so you don't need to think is it 'new
ref' or 'old ref'.
That said that would be good to have some recommendations so people
can use follow them.
True.
Re: [PATCH 8/9] d3drm: Add refcount info to AddRed and Release traces.
Le 09/03/2012 13:06, Nikolay Sivov a écrit :
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c
index 879d8b0..42d722c 100644
--- a/dlls/d3drm/d3drm.c
+++ b/dlls/d3drm/d3drm.c
@@ -87,10 +87,11 @@ static HRESULT WINAPI
IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID
static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface)
{
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI
IDirect3DRMImpl_Release(IDirect3DRM* iface)
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
ULONG ref = InterlockedDecrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref)
HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c
index 559128e..6f9802a 100644
--- a/dlls/d3drm/device.c
+++ b/dlls/d3drm/device.c
@@ -93,10 +93,11 @@ static HRESULT WINAPI
IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2*
static ULONG WINAPI
IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface)
{
IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p)\n", This);
+TRACE("(%p): AddRef from %d\n", This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
I personally don't see a reason to trace actual reference on release
and previous one on AddRef. I usually trace new value in both calls so
you don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name
is visible in traces already.
Indeed that does not make sense but I've a always seen that. In addition
I took the message from another place.
I don't mind changing at all. I like "(%p)->(): new ref = %d\n" and I
will stick to it. That said that would be good to have some
recommendations so people can use follow them.
Re: [PATCH 8/9] d3drm: Add refcount info to AddRed and Release traces.
On 3/9/2012 13:56, Christian Costa wrote:
diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c
index 879d8b0..42d722c 100644
--- a/dlls/d3drm/d3drm.c
+++ b/dlls/d3drm/d3drm.c
@@ -87,10 +87,11 @@ static HRESULT WINAPI
IDirect3DRMImpl_QueryInterface(IDirect3DRM* iface, REFIID
static ULONG WINAPI IDirect3DRMImpl_AddRef(IDirect3DRM* iface)
{
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): AddRef from %d\n", iface, This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM* iface)
@@ -98,7 +99,7 @@ static ULONG WINAPI IDirect3DRMImpl_Release(IDirect3DRM*
iface)
IDirect3DRMImpl *This = impl_from_IDirect3DRM(iface);
ULONG ref = InterlockedDecrement(&This->ref);
-TRACE("(%p/%p)\n", iface, This);
+TRACE("(%p/%p): ReleaseRef to %d\n", iface, This, ref);
if (!ref)
HeapFree(GetProcessHeap(), 0, This);
diff --git a/dlls/d3drm/device.c b/dlls/d3drm/device.c
index 559128e..6f9802a 100644
--- a/dlls/d3drm/device.c
+++ b/dlls/d3drm/device.c
@@ -93,10 +93,11 @@ static HRESULT WINAPI
IDirect3DRMDevice2Impl_QueryInterface(IDirect3DRMDevice2*
static ULONG WINAPI IDirect3DRMDevice2Impl_AddRef(IDirect3DRMDevice2* iface)
{
IDirect3DRMDeviceImpl *This = impl_from_IDirect3DRMDevice2(iface);
+ULONG ref = InterlockedIncrement(&This->ref);
-TRACE("(%p)\n", This);
+TRACE("(%p): AddRef from %d\n", This, ref - 1);
-return InterlockedIncrement(&This->ref);
+return ref;
}
I personally don't see a reason to trace actual reference on release and
previous one on AddRef. I usually trace new value in both calls so you
don't need to keep in mind this +/- 1 offset when looking at logs.
Also you don't need extra words in trace message cause function name is
visible in traces already.
