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

add CPP for ghc-8.4 compatibility, bump version to 0.7.2 #36

Merged
merged 4 commits into from
Mar 12, 2018

Conversation

elaforge
Copy link
Contributor

This lets ghc-events build on ghc 8.4.

This tried to compile but crashed because of the base 4.* constraint. Aren't you supposed to use <= known version for base?

@maoe
Copy link
Member

maoe commented Mar 11, 2018

Could you add GHC 8.4.1 to the Travis build matrix in .travis.yml to see if it actually compiles?

This tried to compile but crashed because of the base 4.* constraint. Aren't you supposed to use <= known version for base?

Agreed. That line was written back in 2009. Could you update it as well? I think >= 4 && < 4.12 should be fine.

@maoe
Copy link
Member

maoe commented Mar 11, 2018

Also it's not a big deal but you don't need to bump the version number nor add a changelog section with the version number. No one knows when the release date will be and the next version can be a major version bump due to other changes. The maintainers do it when s/he is releasing. Maybe we should prepare a CONTRIBUTING.md..

@elaforge
Copy link
Contributor Author

Ok, let me try the travis and cabal changes. One moment.

I bumped the version number because presumably we want to do a release right away. Otherwise any project that depends on this can't use ghc 8.4.

@elaforge
Copy link
Contributor Author

Ok, updated and looks like travis has picked it up. .travis.yml has a bunch of changes, but since it's generated from a script, I'll just trust that it knows what it's doing.

@elaforge
Copy link
Contributor Author

Older ghc versions failed on the bytestring dependency. I don't see why changing the base restriction and adding a new ghc should make old versions fail on bytestring, but maybe it has to do the travis generator changing.

It looks like it supplies bytestring 0.10.0.0, and ghc-events wants >= 0.10.4, but in that case it shouldn't have worked before either.

@elaforge
Copy link
Contributor Author

Ok I think I see. According to https://ghc.haskell.org/trac/ghc/wiki/Commentary/Libraries/VersionHistory, ghc <=7.6.3 ships with bytestring 0.10.0.2, so ghc-events hasn't supported those versions for as long as the bytestring constraint hsa been present, which looks like 7a8b509, by you, on Feb 10 07:19:40 2017.

Likely the reason why the breakage wasn't caught is that the check against installed libraries is new, and is indeed due to make_travis_yml_2.hs being updated and adding a new check. Previously I think it would just install a newer bytestring and pass.

I can either turn off that check, or I can remove ghc <=7.6.3 from tested-with, or you could do some #ifdef to have 7a8b509 only apply when a newer bytestring is present. Turning of the check is a bit sketchy, bytestring is a boot lib and it's hard to say you support a ghc version if it requires overriding a boot lib. But that would be maintaining status quo. Removing the old versions from support seems ok, I think there are not that many users, and I hope if you care about performance you are upgrading ghc. The ifdef stuff seems like too much hassle.

For the short term I'll turn off the INSTALLED check just to make it pass, since the breakage doesn't actually have anything to do with my pull request.

elaforge added a commit to elaforge/ghc-events that referenced this pull request Mar 12, 2018
@maoe
Copy link
Member

maoe commented Mar 12, 2018

I don't have a strong opinion here and am fine either of disabling the installed check or dropping the support for ghc <= 7.6.3. We certainly don't need to employ #ifdef to support that old compilers.

@Mikolaj @kvelicka What do you think?

@Mikolaj
Copy link
Member

Mikolaj commented Mar 12, 2018

I think very few people use 7.6.3 these days and if they do, they don't need 8.4 compatibility, so they can stay at old ghc-events versions, so I'd drop 7.6.3. BTW, great job @elaforge.

@kvelicka
Copy link
Contributor

+1 for removing the constraint, same reasoning as Mikolaj.

@maoe
Copy link
Member

maoe commented Mar 12, 2018

Okay. Let's drop the support for ghc <= 7.6.3 then. @elaforge Could you please update the PR?

It turns out those versions have an older bytestring than ghc-events
requires since  7a8b509, and the new
travis config complains about overriding bootlibs.  Details in PR haskell#36.
@elaforge
Copy link
Contributor Author

Updated and travis is happy now.

Can I get someone to do a hackage release after merging? My main goal is to get my project compiling with 8.4. I could do the release but I probably don't have upload privileges.

@maoe
Copy link
Member

maoe commented Mar 12, 2018

Thanks! I can merge this and upload a new version in a few hours.

@maoe
Copy link
Member

maoe commented Mar 12, 2018

Looks perfect! Thank you again for your work.

@maoe maoe merged commit 7c49274 into haskell:master Mar 12, 2018
@maoe
Copy link
Member

maoe commented Mar 12, 2018

@maoe
Copy link
Member

maoe commented Mar 12, 2018

@elaforge May I ask how you use this library in your project if you don't mind? I'm curious about usage patterns of the library beside ThreadScope and ghc-events-analyze.

@elaforge
Copy link
Contributor Author

I have a converter that writes JSON read by chrome://trace. This is like threadscope, but has more features and is already installed wherever you happen to have chrome. Currently I use it for my own project, but eventually I want to generalize and put on hackage.

@maoe
Copy link
Member

maoe commented Mar 13, 2018

That sounds cool. How is the memory usage of the event viewer? Can you look into a huge eventlog file without a memory hog?

I'm meaning to implement some memory-efficient indexing in ghc-events and use it from ThreadScope to fix the memory hog.

@elaforge
Copy link
Contributor Author

elaforge commented Mar 13, 2018

I haven't tried it with large traces yet. I'm guessing chrome can handle pretty big ones, because it's primary use is debugging performance issues in web pages down to low level rendering, and those generate a lot of events.

I'm pretty sure it still loads the whole file into memory at once though, so if by huge you mean won't fit in RAM, than it probably can't handle that.

Still, a streaming interface to ghc-events would be nice, so I could convert a file without having to load it all into memory. I've been using streaming on hackage, and it's pretty straightforward.

@maoe
Copy link
Member

maoe commented Mar 13, 2018

A quick google search turned up golang/go#11520, which is unresolved yet.

Still, a streaming interface to ghc-events would be nice, so I could convert a file without having to load it all into memory.

I guess this is doable today, isn't it? Thanks to @kvelicka, we have a streaming interface. I have no idea what the trace format in Chrome looks like but if you don't need to sortEvents, you should be able to convert an eventlog in constant space.

Anyway, please let me know when you publish your work. I have huge eventlogs around so I can test.

@elaforge
Copy link
Contributor Author

Ah so there is. It should be easy to plug that interface into a stream. Chrome has some restrictions about sortedness, but also some freedom, it depends on the event type. In fact it might be the same semi-sorted that GHC produces naturally... something like sorted within a thread, but no guarantee across threads? I thought I read somewhere it uses per-thread buffers that causes that.

For large eventlogs, I planned to have a heuristic to find interesting ranges, such as ones that exceeded a latency threshold, and emit those separately. That would avoid the need to load the whole thing in one go, and reduce the need to scroll and zoom and try to find interesting things by eye.

I will announce when I have something usable.

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.

None yet

4 participants