Add linker map and .def file #197

igchor · 2024-01-31T21:52:42Z

No description provided.

.github/workflows/basic.yml

src/umf.def

cmake/helpers.cmake

lukaszstolarczuk

👍

lukaszstolarczuk · 2024-02-02T09:31:29Z

.github/workflows/basic.yml

        include:
          - os: 'windows-2022'
            build_type: Release
            compiler: {c: clang-cl, cxx: clang-cl}
            pool_tracking: 'ON'
+            os_provider: ['ON']


['ON'] -> 'ON' 😉

not quite - pls only update in includes, not in matrix (a list is required there)

src/libumf.def

src/libumf.map

bratpiorka · 2024-02-02T17:54:28Z

it would be really great if you split this PR to smaller single-commit PRs

igchor · 2024-02-02T18:24:49Z

it would be really great if you split this PR to smaller single-commit PRs

Ok, done:
#200
#201

bratpiorka · 2024-02-05T09:08:35Z

@igchor please rebase + change the name of this PR + could you also extract the changes in the github action file into a separate PR?

igchor · 2024-02-05T17:57:19Z

@igchor please rebase + change the name of this PR + could you also extract the changes in the github action file into a separate PR?

I'll do that once #200 is merged

igchor · 2024-02-05T18:41:41Z

@igchor please rebase + change the name of this PR + could you also extract the changes in the github action file into a separate PR?

Done - rebased and changed the name of the PR. I've left the github action changes though since they are mostly to make sure that adding .def/.map file doesn't break anything.

bratpiorka · 2024-02-06T08:08:13Z

@igor please resolve the issue pointed by @lukaszstolarczuk
also, I think it would be good to generate def/map files in the future and add a test for that - I will create a JIRA task for this

lukaszstolarczuk · 2024-02-06T09:39:03Z

besides the issue (mentioned by Rafał) blocking CI, lgtm

igchor · 2024-02-06T18:07:57Z

@igor please resolve the issue pointed by @lukaszstolarczuk also, I think it would be good to generate def/map files in the future and add a test for that - I will create a JIRA task for this

Done.

igchor · 2024-02-06T19:55:15Z

Hm, there's now another problem after we started using base alloc in jemalloc/scalable pool implementations. For dynamic library we should hide base_alloc symbols from libumf.so but that means we would need to build jemalloc/scalable pool with base_alloc sources (same as tests in: #200).

However, for static libumf, we do not control symbol visibility so if both libumf.a and jemalloc/scalable_pool.a are build with base_alloc we'll have conflicting symbols I think. Any ideas how we can solve this? Should we make base_alloc a separate dynamic library?

bratpiorka · 2024-02-08T11:27:26Z

Hm, there's now another problem after we started using base alloc in jemalloc/scalable pool implementations. For dynamic library we should hide base_alloc symbols from libumf.so but that means we would need to build jemalloc/scalable pool with base_alloc sources (same as tests in: #200).

However, for static libumf, we do not control symbol visibility so if both libumf.a and jemalloc/scalable_pool.a are build with base_alloc we'll have conflicting symbols I think. Any ideas how we can solve this? Should we make base_alloc a separate dynamic library?

can't we check for this case in cmake? and do not include base_alloc sources in static jemalloc/scallable pools? These libs will never be used without libumf.a

On linux, we haven't limited symbol visibility in any way - this patch does that by introducing linker map. On windows, this patch replaces UMF_EXPORT with .def file to keep headers clean.

To verify building with OS memory provider turned off

if umf is build as a shared lib. This is needed because ba_alloc symbols are not longer exported from libumf

igchor · 2024-02-08T18:55:16Z

Hm, there's now another problem after we started using base alloc in jemalloc/scalable pool implementations. For dynamic library we should hide base_alloc symbols from libumf.so but that means we would need to build jemalloc/scalable pool with base_alloc sources (same as tests in: #200).
However, for static libumf, we do not control symbol visibility so if both libumf.a and jemalloc/scalable_pool.a are build with base_alloc we'll have conflicting symbols I think. Any ideas how we can solve this? Should we make base_alloc a separate dynamic library?

can't we check for this case in cmake? and do not include base_alloc sources in static jemalloc/scallable pools? These libs will never be used without libumf.a

Ok, done. I've also had to add ba_alloc init code to jemalloc and scalable pools (see topmost commit). Still, this is not an ideal solution since our jemalloc/scalable pool libs will only work with libum that they were build with (.e.g. if jemalloc_pool.a was build with libumf.so it won't work with libumf.a).

Since ba_alloc sources are compiled directly into those pools they each have a separate copy of base_alloc.

bratpiorka · 2024-02-09T08:10:28Z

Hm, there's now another problem after we started using base alloc in jemalloc/scalable pool implementations. For dynamic library we should hide base_alloc symbols from libumf.so but that means we would need to build jemalloc/scalable pool with base_alloc sources (same as tests in: #200).
However, for static libumf, we do not control symbol visibility so if both libumf.a and jemalloc/scalable_pool.a are build with base_alloc we'll have conflicting symbols I think. Any ideas how we can solve this? Should we make base_alloc a separate dynamic library?

can't we check for this case in cmake? and do not include base_alloc sources in static jemalloc/scallable pools? These libs will never be used without libumf.a

Ok, done. I've also had to add ba_alloc init code to jemalloc and scalable pools (see topmost commit). Still, this is not an ideal solution since our jemalloc/scalable pool libs will only work with libum that they were build with (.e.g. if jemalloc_pool.a was build with libumf.so it won't work with libumf.a).

ok, thanks. About the scnario you mentioned - currently there is no way to build only jemalloc_pool.a without building libumf. So I think the problem here is very limited

igchor requested a review from a team as a code owner January 31, 2024 21:52

igchor mentioned this pull request Jan 31, 2024

Add proxy_pool #185

Merged

igchor changed the title ~~Add linker map and .def file~~ Add linker map and .def file and expose ops through getter functions Jan 31, 2024

lukaszstolarczuk reviewed Feb 1, 2024

View reviewed changes

.github/workflows/basic.yml Show resolved Hide resolved

src/umf.def Outdated Show resolved Hide resolved

cmake/helpers.cmake Outdated Show resolved Hide resolved

ldorau mentioned this pull request Feb 1, 2024

Add proxy pool to benchmark #198

Merged

ldorau approved these changes Feb 1, 2024

View reviewed changes

igchor force-pushed the windows branch from aa9015b to b7236ec Compare February 1, 2024 16:26

ldorau approved these changes Feb 1, 2024

View reviewed changes

PatKamin approved these changes Feb 2, 2024

View reviewed changes

lukaszstolarczuk reviewed Feb 2, 2024

View reviewed changes

lukaszstolarczuk requested changes Feb 2, 2024

View reviewed changes

DamianDuy reviewed Feb 2, 2024

View reviewed changes

src/libumf.def Outdated Show resolved Hide resolved

igchor force-pushed the windows branch from b7236ec to bd49770 Compare February 2, 2024 16:12

bratpiorka reviewed Feb 2, 2024

View reviewed changes

src/libumf.map Show resolved Hide resolved

igchor force-pushed the windows branch from bd49770 to 655bec1 Compare February 5, 2024 18:40

igchor changed the title ~~Add linker map and .def file and expose ops through getter functions~~ Add linker map and .def file Feb 5, 2024

ldorau approved these changes Feb 6, 2024

View reviewed changes

ldorau requested a review from lukaszstolarczuk February 6, 2024 08:13

igchor force-pushed the windows branch from 655bec1 to 158009c Compare February 6, 2024 18:07

Use linker map and .def file

580a287

On linux, we haven't limited symbol visibility in any way - this patch does that by introducing linker map. On windows, this patch replaces UMF_EXPORT with .def file to keep headers clean.

igchor added 2 commits February 8, 2024 19:20

Add extra windows run

c418354

To verify building with OS memory provider turned off

Add ba_alloc sources to pool libs

0fb7119

if umf is build as a shared lib. This is needed because ba_alloc symbols are not longer exported from libumf

igchor force-pushed the windows branch from 158009c to 51d9aa4 Compare February 8, 2024 18:53

igchor force-pushed the windows branch from eee9aa7 to 35b36c6 Compare February 8, 2024 19:25

igchor added 2 commits February 8, 2024 20:36

Initialize ba_alloc_global in every pool that is build as a separate lib

0c55d8f

Since ba_alloc sources are compiled directly into those pools they each have a separate copy of base_alloc.

Add lib threads to umf_utils

9b96f8b

igchor force-pushed the windows branch from 35b36c6 to 9b96f8b Compare February 8, 2024 19:36

igchor mentioned this pull request Feb 8, 2024

Add abstraction on top of ba_alloc which supports arbitrary size allocations #203

Merged

lukaszstolarczuk approved these changes Feb 9, 2024

View reviewed changes

bratpiorka merged commit 310dbe1 into oneapi-src:main Feb 9, 2024

igchor deleted the windows branch February 9, 2024 16:13

Add linker map and .def file #197

Add linker map and .def file #197

Uh oh!

Conversation

igchor commented Jan 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

igchor Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

igchor Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bratpiorka commented Feb 2, 2024

Uh oh!

igchor commented Feb 2, 2024

Uh oh!

bratpiorka commented Feb 5, 2024

Uh oh!

igchor commented Feb 5, 2024

Uh oh!

igchor commented Feb 5, 2024

Uh oh!

bratpiorka commented Feb 6, 2024

Uh oh!

lukaszstolarczuk commented Feb 6, 2024

Uh oh!

igchor commented Feb 6, 2024

Uh oh!

igchor commented Feb 6, 2024

Uh oh!

bratpiorka commented Feb 8, 2024

Uh oh!

igchor commented Feb 8, 2024

Uh oh!

bratpiorka commented Feb 9, 2024

Uh oh!

Uh oh!