Skip to content
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

Force RAII to solve leaks on ubuntu - fixes #924 #1065

Closed
wants to merge 2 commits into from
Closed

Force RAII to solve leaks on ubuntu - fixes #924 #1065

wants to merge 2 commits into from

Conversation

etcimon
Copy link
Contributor

@etcimon etcimon commented Apr 9, 2015

For some obscure reason, the Ubuntu platform that was leaking requires this.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 9, 2015

It looks like build is failing because the curl dmd 2.067 download fails with error code 22

@s-ludwig
Copy link
Member

Hm, two questions:

  1. Do you know why the FreeListRef failed? After all, it also uses RAII to free the memory.
  2. I kept VibeNoSSL as it is, so that no existing projects get broken. If we want to switch to VibeNoTLS, VibeNoSSL should still be recognized as version(VibeNoSSL) version = VibeNoTLS;. May be then later there can also be a deprecation warning, before finally removing it.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

Ok yes, a compatibility deprecation would do. I couldn't investigate further for lack of time, it looks like an edge case.

I'm currently writing some high level tests to check new client/server features

Both http/2 and botan are done now, but dmd32 goes out of memory on win64 :/

@s-ludwig
Copy link
Member

Yeah, 2.067 got a lot worse wrt memory use :(

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

Wasn't there a flag or an option in dub to compile each file individually?

@s-ludwig
Copy link
Member

Yes, --build-mode=singleFile, possibly with --parallel if the machine has enough RAM.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

Yes, --build-mode=singleFile, possibly with --parallel if the machine has enough RAM.

Just tried that, looks like it's not an option. I get errors in every file regarding the static if I'm using to add/remove features. I think Win64 & DMD will be off the table, I'll probably look towards LDC for the near future.

Well now since openssl and botan don't go together, the dub json is going to be a little big:

https://github.com/etcimon/vibe.d/blob/a5f43cfb2fb2db4fbc6a7007f1d8a8cf2b63f59e/dub.json

The 32mscoff is necessary for windows, but the libs aren't included with the compiler. The DMD64 doesn't exist on windows. I won't know what to do with this branch after I'm done testing it

@s-ludwig
Copy link
Member

Someone should write a little DEP to add "features" in addition to "configurations". But why do you need all of these? Couldn't you use platform suffixes instead?:

        {
            "name": "libasync_botan_posix64",
            "dependencies": {
                "botan": {"version": "~>1.11.11" },
                "libasync": {"version": "~>0.7.2" }
            },
            "versions": ["Botan", "VibeUseNativeDriverType", "VibeLibasyncDriver"]
        },
        {
            "name": "libasync_botan_posix32",
            "dependencies": {
                "botan": {"version": "~>1.11.11" },
                "libasync": {"version": "~>0.7.2" }
            },
            "versions": ["Botan", "VibeUseNativeDriverType", "VibeLibasyncDriver"],
            "subConfigurations": { "botan": "full_x86" }
        },
        {
            "name": "libasync_botan_win32",
            "dependencies": {
                "botan": {"version": "~>1.11.11" },
                "libasync": {"version": "~>0.7.2" }
            },
            "versions": ["Botan", "VibeUseNativeDriverType", "VibeLibasyncDriver"],
            "dflags-windows-x86": ["-m32mscoff"],
            "subConfigurations": { "memutils": "32mscoff", "libasync": "32mscoff", "botan": "full_win32", "libhttp2": "32mscoff" }
        },

Also, why is 32-bit COFF needed? If there is no technical requirement, I'd leave that as a user decision. Same goes for VibeUseNativeDriverType, that should also be a user option.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

I tried subConfigurations-windows-x86 but it's not implemented.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

Also, why is 32-bit COFF needed? If there is no technical requirement, I'd leave that as a user decision. Same goes for VibeUseNativeDriverType, that should also be a user option.

There's more than 32k symbols if you count in the botan library, so unfortunately OMF isn't an option

@s-ludwig
Copy link
Member

Sorry, you are right about subConfigurations. Forgot about that. But can't you structure the botan package description in a way that it doesn't require explicit configuration selections?

More than 32k symbols in botan alone? Shoudn't the individual packages each more or less have their own share of available symbols (except for template instantiations)?

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

More than 32k symbols in botan alone? Shoudn't the individual packages each more or less have their own share of available symbols (except for template instantiations)?

D doesn't have a pre-processor, so I ended up replacing all the ASM and SIMD code generation macros into CTFE functions. That stuff takes up so much symbol space it's hard not to cringe.

e.g. https://github.com/etcimon/botan/blob/master/source/botan/hash/sha1_x86_64.d#L268

But can't you structure the botan package description in a way that it doesn't require explicit configuration selections?

Yes, I just realized I can use version-x86_64 and version-x86, so the only thing is the coff. If it had a dflags-x86-coff it would be possible not to split configurations, assuming the --arch=32mscoff gets propagated correctly (which is the nature of my subConfigurations hack)

However, for openssl/botan combinations I can't do much.

@s-ludwig
Copy link
Member

Right, OpenSSL and Botan should both be handled by "features" rather than "configurations", but for now that should be ok.

About Botan, would separating the library into multiple sub packages be an option or do the modules have too many interdependencies?

@etcimon
Copy link
Contributor Author

etcimon commented Apr 14, 2015

About Botan, would separating the library into multiple sub packages be an option or do the modules have too many interdependencies?

I haven't looked into subpackages yet, but there's a lot of "version" tags that allows to start with a very light bare bone (sha256 gives me a 800kb exec), and you can add algorithms selectively. You can see the dub file here. I'm not sure if defaulting to a lower setting is easily do-able, the factory finds algorithms with string matching so the compiler can't give any warnings. If the algorithm isn't versioned in when it's requested by a user application, it will crash with a segfault, and most of the unit tests aren't designed for this selective use case.

This is probably alright for the TLS general purpose, so I'd have to give that a try.

@dariusc93
Copy link
Contributor

@s-ludwig Could you explain what you meant when you said that 2.067 got worse with memory?

@s-ludwig
Copy link
Member

I have a few projects that don't compile on Windows anymore due to out of memory errors. DMD 2.066.1 still works, though.

@etcimon
Copy link
Contributor Author

etcimon commented Apr 16, 2015

I've managed to make a dmd x64 build on windows thanks to this:

http://forum.dlang.org/thread/mghqlf$10l2$1@digitalmars.com#post-ybrtcxrcmrrsoaaksdbj:40forum.dlang.org

@s-ludwig
Copy link
Member

All issues with FreeListRef have been fixed, so this can be closed safely.

@s-ludwig s-ludwig closed this Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants