Re: [PATCH 2/5] wined3d: Wrap resource->heapMemory into a structure
On 19 August 2013 10:52, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
+struct wined3d_heap_memory *wined3d_alloc_resource_mem(UINT size) +{ + struct wined3d_heap_memory *ret; + + ret = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*ret)); + if (!ret) + return NULL; + + ret->alloc_ptr = HeapAlloc(GetProcessHeap(), 0, size + RESOURCE_ALIGNMENT); + if (!ret->alloc_ptr) + { + HeapFree(GetProcessHeap(), 0, ret); + return NULL; + } + ret->ptr = (BYTE *)(((ULONG_PTR)ret->alloc_ptr + + (RESOURCE_ALIGNMENT - 1)) & ~(RESOURCE_ALIGNMENT - 1)); + return ret; +} +
I don't think this is a good idea. How about something like the following? The corresponding free function would be trivial. void *wined3d_memalign(SIZE_T alignment, SIZE_T size) { SIZE_T align; void *mem; void **p; align = max(sizeof(*p), alignment - 1); if (!(mem = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size + align))) return NULL; p = (void **)(((ULONG_PTR)mem + align) & ~(alignment - 1)) - 1; *p = mem; return ++p; } (Untested, there's probably some room for improvement.)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-08-19 12:29, schrieb Henri Verbeet:
I don't think this is a good idea. How about something like the following? The corresponding free function would be trivial. I guess this works for the purposes of this patchset. However, in my command stream patches I am using a structure similar to this to cache allocated blocks of memory to handle WINED3D_MAP_DISCARD buffer maps. In that case the structure had additional size information and a struct list entry.
I guess in theory any kind of payload could be added to your implementation, but growing this larger than RESOURCE_ALIGNMENT(=16) bytes moots the point I guess. I'm not yet sure though if my cache is an actual improvement over just freeing and allocating new blocks of memory. The other unknown is that some ddraw games write past the allocated surface memory. The extra 16 bytes took care of this, and before we aligned the memory we just allocated 4 extra bytes. I don't know which games are affected by this (it was from a comment in old ddraw), and I don't know if the problem only happened if the memory block wasn't properly aligned. Your code and mine differ when HeapAlloc happens to return an aligned pointer. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSEgO5AAoJEN0/YqbEcdMwmiIP/iVETzzMWT/yDSmFNSOAD3o5 59TXEer+KeWUH/tVUr+WA6vxnT3CR18hV7LZl9fPWt9d3pRS12zyYSIz5i/5f9eJ BgK15qFFrZhzFu8c1hzYIhM76nUtInPMVi3zd/7xkgUI3aemuP0dUkJIYd+wrUcz 4S50ppMIgJhqGReFIdTiOz57OPQWUFUoBKDxFlv3WSKgV8b1it99IwXgX/IPuvbQ cP6REIW1WtI6dLvOjoT6D8v2yUSmNmSDAMVBx4D7LEBhVTmxPMuFjbn4t5UJ1+wY wpr2mBNGdiwaLU+XQA5X1UrHDTYPDEQS0pJ9hub8sotwl+QiXWrcTNtw5avai3Xy uztAF7kkCNQpbHopp2GjHXQaGxGoKyXQTI1okFd/R1C/9q8TOCA1gG8c/Ntx9Vhd ORG+HCUr+x+coPfRBwgxdl/22zLHSotVXhmT+2r4uoklnZN+0TX0AKtctwIGBMpR 0Kw9fnBPngpmEHheTDhr94nqZL3FIBLyqRDrdWxiy0zmDu1hShcC5/ox8MutevaR G4bXHlLc4BbW+PN6/wJDNCG3Nt/avdwU0rj4Ix6c7X/Cns+kmyWECiCjRkYo4xQK nlr8wDZC307fcrIH6jfh7DqdPhcMKA17LeKRm6ygpDp5sYqpsTb4r7NOZXJA6h63 nK3qN+FJ4PEbK90d+tg4 =sh5J -----END PGP SIGNATURE-----
On 19 August 2013 13:38, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
I guess in theory any kind of payload could be added to your implementation, but growing this larger than RESOURCE_ALIGNMENT(=16) bytes moots the point I guess. The point is mostly that I don't think we want the wined3d_heap_memory structure to need a separate allocation. I suppose you could also just embed it inside the wined3d_resource structure.
properly aligned. Your code and mine differ when HeapAlloc happens to return an aligned pointer.
That's actually a bug in what I posted.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-08-19 13:53, schrieb Henri Verbeet:
On 19 August 2013 13:38, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
I guess in theory any kind of payload could be added to your implementation, but growing this larger than RESOURCE_ALIGNMENT(=16) bytes moots the point I guess. The point is mostly that I don't think we want the wined3d_heap_memory structure to need a separate allocation. I suppose you could also just embed it inside the wined3d_resource structure. Storing the pointer inside the wined3d_resource structure won't work for the command stream, but I'll ignore those considerations until I send actual CS patches and know for sure that my cache is an actual improvement.
properly aligned. Your code and mine differ when HeapAlloc happens to return an aligned pointer.
That's actually a bug in what I posted. What's your suggested fix? Allocating size + align + alignment, and adding alignment to mem if mem happens to be already aligned?
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSEg2mAAoJEN0/YqbEcdMwOEoP/iWfAY48+3DnewhUbKbuN2L/ 1hy4E/4/66lQYn/Zjh0OppmyyKYbVmphUGMFOsoVEgMr2pXuoorkoixaSR/gA2k2 8UBKHxKahRUIIfHrUUiHopD4WqN7D0RNFppPF7kc3o/lrFUgTGSbTCSbu4+1v/1S 0Bku9tQ+0pG4zv5aHivtXNUcZeEmK+8NjXwk9HOL4ro6Eg8by1QpSW/ZdsnsPWci aH8HhfShQN6jmGfe04r2L/dP/EILhsNgJ8bRzrxi48M+IHX8jUdX10bwlGYy9BRi 89YvxFXb/jyHk2NdpYfHjntDLy1wWs/tMI5F9ns/p95L6VVSnKQUROMFz4iyRrT8 vJRmeiXeIFTyMLs+XfXioDzZTh6+iMtnmuCk/xTH4pzTeMWJTosJs2vcWQVf5aka SF5fFxg3HuT3VNFqql5s5oTKXJ1MiB3sDkPHgo6s68AS/TpCFicCUca+WPHfriQO FrW7kr/wUe9Krzh4bQQQt8SB5rJJWVYYXNPV9A+BOAZzSAbGMMXIi2QVTb9FHDu5 znspnnZbHUs4fyehJpzCQEwR1WcAkaEVT9t9owNefxvwrZhbGq4GTd8YwNtr61UL xkixy8z4cA0ul1lW/QGpBMswtYYBKa1m43HzLcZ+1sqtftLkgApDbOxW5BCMdDLO 56TMnG+BJCGaRgQVzoov =nHsF -----END PGP SIGNATURE-----
On 19 August 2013 14:20, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2013-08-19 13:53, schrieb Henri Verbeet:
That's actually a bug in what I posted. What's your suggested fix? Allocating size + align + alignment, and adding alignment to mem if mem happens to be already aligned?
I think it needs to be "align = max(sizeof(*p), alignment);", but that's actually for the aligned + 1 case. I think we can always add some extra padding in the caller if it turns out to be needed, but I'm tempted to not worry too much about it, since it could very well be that the original comment was actually about alignment, and a regression test should give pretty obvious results if this breaks something.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-08-19 14:34, schrieb Henri Verbeet:
On 19 August 2013 14:20, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
Am 2013-08-19 13:53, schrieb Henri Verbeet:
That's actually a bug in what I posted. What's your suggested fix? Allocating size + align + alignment, and adding alignment to mem if mem happens to be already aligned?
I think it needs to be "align = max(sizeof(*p), alignment);", but that's actually for the aligned + 1 case. With some inspiration from _aligned_offset_malloc in msvcrt I came up with this:
void *wined3d_alloc_resource_mem(SIZE_T size) { void **p; SIZE_T align = RESOURCE_ALIGNMENT - 1 + sizeof(*p); void *mem; if (!(mem = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size + align))) return NULL; p = (void **)(((ULONG_PTR)mem + align) & ~(RESOURCE_ALIGNMENT - 1)) - 1; *p = mem; return ++p; } void wined3d_free_resource_mem(void *mem) { void **p = mem; if (!mem) return; HeapFree(GetProcessHeap(), 0, *(--p)); } I'm not entirely sure about the SIZE_T align = RESOURCE_ALIGNMENT - 1 + sizeof(*p); line though. Msvcrt doesn't have the - 1, and allocating size + RESOURCE_ALIGNMENT bytes might be enough. msvcrt allocates the equivalent of size + RESOURCE_ALIGNMENT + sizeof(void *). -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSEjTlAAoJEN0/YqbEcdMwHhsP/3jldV7GHrhg8Asj1wxksNbz vXorIHDwBQQQFmcj4d5o7zyXsMho/SfebMeesmtJCtmfZeOasWjE02vgnbZSbU1T OA4Aow7orGeGWUCYddtJYLoDwC4HSaIXb/8HO5rCNnM1tVboHn0XXBM5q24t+fZo jbmX6Ma6QtD/b6dV+TCvh8YUih5Yy/lTr0Fq1c3nrodz7JTbHqu7qfdpK2QPqB4P ekRtHofPTLKLAxwZFw5Vc5hdUZ1xfejUuKgzQWnPFbb/jHFdN9gC+2mZdn0YbyEs hATXQX8N2oakjgzkBg1eyZInhPLHFTNrz5EfR0tolv+ytXdy0YfcOjSYkYOODkNb bUr1l1uTBw2trVYFtx52XqkhuoXZhj2DAbGtM2vmd81Sttce1CnZd08jcedDllon sSZ28lkvwxVL6N6pNkrpTWjaKj3RcaAhqJm5zj+MHX95w8gPl6oFps1lCrLXVaDP gsC9FeHL3QPf7Hb5arJvcxbH17CVRi+i4irJIgnOgJMLkZGK3+jbadKHPHA0D2i8 lTewyhf70+FJd04yZ0yoiXTYcJbZU9BkQMKxbTpywoUP8+3ZJ3n7PMe+CoFyphr1 fzZFR0sV/T4kCLBbH8YeTsiWluB0ioOCsh9cyNZrz6C2Dy5PmNmwTN0dWpi7LukF tamCuVqBu//pY5YUuq9d =7d25 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2013-08-19 17:08, schrieb Stefan Dösinger: > I'm not entirely sure about the SIZE_T align = RESOURCE_ALIGNMENT - > 1 + sizeof(*p); line though. Msvcrt doesn't have the - 1, and > allocating size + RESOURCE_ALIGNMENT bytes might be enough. msvcrt > allocates the equivalent of size + RESOURCE_ALIGNMENT + sizeof(void > *). After going through all the possibilities I concluded that size + alignment + sizeof(void *) - 1 is the smallest allocation size. The worst case is a pointer of the form n * alignment - sizeof(void *) - 1. In this case we need sizeof(void *) - 1 bytes to get to the next aligned address and then alignment bytes to store our real alloc pointer. Not sure why msvcrt allocates one extra byte. Probably because allocations below 4 or 8 bytes don't matter anyway. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSEjl3AAoJEN0/YqbEcdMw/JAP/iw92FaWRGOMoOMToYTuBqyC K9xgwOXxPuViICgR92uAMv1K/vn+38WWmuTULGKjRx9qdddOBEuYd2cIS1xUjLub laegv8EIxn/g426xqjZ8mSqXBrFyHMhGVTREuWbr4G4yIH2CB+yzmIg7IObgaPoS G9Gleeg0+pjzyPnqdCy9NWu/HEWDKVQ4Zdqms2oBmgfpPNtSblwh/BFeoIf5+eON 4hzIcY+rYKK0prGaJ7GEVFIocLqZJuF4Z6KiDS+jg4jRx6pCXQ+m1GcNDb90fYZX i8nWO2TfVmTZ8j5yb3Z5YTW5C/3UIQDJhBlKCqZyu2FhR+PTDzTEnWow9fp1kfCK KHZYcJmomSRQX+xWinjKAqatzUtsM2HbT+cyUeiQKVVpSNOjSFCM6tzsT0q9/PQd 89G4kfPIwArKyNHffNXeiuEWTKYY4/a3E5/E4dbIFxVvxrqxaeg5jDuG6Ic0LQK/ IEAfJHH5LLwM6Ef3s3lWmkgRTMYE1oWyys5uP0Ht3C0xUoomohpXQrrgRZNfTjtL J49rvEQ+yZKPPrXOqjLtIwKFC/mdb3/4x33b2UMob5HRN7Jjusvg7OefqYmOGprr UFB0iIwg7oOuqThZ+Ce5jnu5p0vvvPYq1vEuW7Oz/wth+aENibNatvKnJFj/tWOe i40GHsaztVcRtNGbZsap =zZYQ -----END PGP SIGNATURE-----
participants (3)
-
Henri Verbeet -
Stefan Dösinger -
Stefan Dösinger