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 Source Map support. #1693

Merged
merged 2 commits into from Feb 24, 2016
Merged

Add Source Map support. #1693

merged 2 commits into from Feb 24, 2016

Conversation

@nwolverson
Copy link
Contributor

nwolverson commented Dec 6, 2015

Per #121. Seems to work decently for me now testing via https://github.com/lydell/source-map-visualize

  • Needs cleaned up
  • Need to put behind a command line flag
  • Not sure if there are performance implications, could be bad. Seemed super slow to build my project just now but need to compare against master
  • Could have broken other things by introducing new nodes in the AST, could this prevent some optimisation from firing?
  • For best effect psc-bundle would need to support, not sure how hard that is

Did manage to combine these source maps for a larger app with webpack (custom webpack config), I'm getting a runtime error which may or may not be related to my changes.

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 6, 2015

Worse than I thought, this is not going to work:

real 0m11.382s
user 0m27.428s
sys 0m4.606s

real 3m55.869s
user 10m27.173s
sys 2m52.620s

@paf31
Copy link
Contributor

paf31 commented Dec 6, 2015

Very interesting. Do you find the source maps useful for debugging? Maybe it's possible to fix the speed issue.

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 6, 2015

Well in general I find source maps useful. e.g. I've used with coffescript, TypeScript, babel, even though you can get away fine without it. Particularly when you have 1 combined output file. I think having clean/straightforward output as per the PureScript design should also mean that source maps work better anyway.

Not had time yet to really experience this implementation since the point it actually worked in a usable way.

I presume some (lesser) performance hit is acceptable when the option is actually enabled but I suspect most of this is encountered at the JS pretty-print stage.

@garyb
Copy link
Member

garyb commented Dec 6, 2015

I've actually not yet encountered a situation where source maps would be helpful in PureScript - the difference with all the languages you list is they have unchecked runtime exceptions 😉

I think I may have been the one who suggested source map support originally, so I'm not at all against it, but just thought I'd mention that although they're nice to have I think their utility is perhaps not what I expected - probably their most useful aspect is a selling point for JS devs who know little about PS, that think source maps are required for transpiled JS to be practical.

@garyb
Copy link
Member

garyb commented Dec 6, 2015

I assume this is the embedded sourcemap style? Perhaps we could get better performance out of generating the separate map file kind instead?

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 7, 2015

I actually use dev tools a lot to understand program flow and logic errors when there is no run time error, rather than printf debugging (which eg I did a lot of in preparing this PR as I didn't invest the time to see what better tools are available).

I was wrong my guess, seems like the perf problem is in actual source map generation, adding a flag and turning off the actual generation just gave 13.7s time. The maps are not the embedded style. I guess this means the issue is likely in the source-maps library, it really shouldn't be that expensive.

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 8, 2015

Found the major issue in sourcemaps, with that change:

real 0m13.572s
user 0m34.961s
sys 0m6.025s

@@ -70,7 +70,8 @@ library
process >= 1.2.0 && < 1.4,
safe >= 0.3.9 && < 0.4,
semigroups >= 0.16.2 && < 0.19,
parallel >= 3.2 && < 3.3
parallel >= 3.2 && < 3.3,
sourcemap >= 0.1.3

This comment has been minimized.

Copy link
@paf31

paf31 Dec 11, 2015

Contributor

You'll need to rerun the license-generator executable.

@paf31
Copy link
Contributor

paf31 commented Dec 11, 2015

Not bad at all 😄

What was the issue?

@paf31
Copy link
Contributor

paf31 commented Dec 11, 2015

Does this change affect the speed when source maps are not generated?

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 11, 2015

Issue in sourcemaps, fixed in latest version 0.1.5: chrisdone/sourcemap@00e776b

Still some performance impact outside of that, including without sourcemaps being generated, was looking at some profiling but have been laid up with a cold.

Can't for the life of me get cabal-dependency-licenses to run.

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 20, 2015

Original (median of 3 runs with above project, which does generate about 500 errors)

real 0m11.173s
user 0m27.750s
sys 0m4.486s

With changes, source-maps not enabled (generally seeing 5-10% hit, and I can't seem to improve it):
real 0m12.161s
user 0m29.225s
sys 0m4.968s

With sourcemaps:
real 0m15.844s
user 0m39.553s
sys 0m7.191s

Turns out adding an intermediate node in the JS AST for position information broke both optimisation and correct parenthesisation, so should maybe have bitten the bullet and added to each constructor. If it's worth continuing with this.

@garyb garyb mentioned this pull request Dec 31, 2015
@paf31
Copy link
Contributor

paf31 commented Dec 31, 2015

A thought on performance: if we had two separate code generators, one for source maps and one for JS, we could choose to run one or both. But obviously we need information from the JS generator to construct the source maps.

But what if we wrote one polymorphic code generator:

class (Monoid gen) <= Emit gen where
  emit :: String -> gen

codegen :: (Emit gen) => JS -> gen

(approximately) then give instances of Emit for String (JS) and some counting data structure (source maps). With some inlining and specialization, that might get close to the performance of separate generators.

Just a thought, but maybe one for another PR. It would definitely be nice to have zero-cost when not using source maps though.

@nwolverson
Copy link
Contributor Author

nwolverson commented Dec 31, 2015

That does look nicer than trying to build a data structure that works conditionally for both cases as I've done. I wasn't clear where any remaining performance issue for the non-map case was, the only things that I noticed in profiling seemed like they should be an issue already (bunch of string concatenation), I thought maybe there was some effect from additional AST nodes that were inserted pre-codegen.

(I lack Haskell experience to have any good feel for performance issues, style etc...)

@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from 08d248a to d3e347d Jan 30, 2016
@nwolverson
Copy link
Contributor Author

nwolverson commented Jan 30, 2016

So I redid this to use the Emit class suggested, this seems to all get specialised and inlined OK. Also removed the extra JSPositioned constructor and folded in the sourcespan to each constructor, still some cruft due to that but can at least be sure the optimiser doesn't get broken. Seeing pretty good performance with source maps off, I disabled all error messages (because they were the majority of the build time!) and ran on the same largish project, couldn't really distinguish performance vs master over the variance between runs.

Still needs work like ensuring a good set of positions are outputted (but some of that is maybe due to bad sourcespans coming from earlier in the compilation process).

@@ -126,120 +130,190 @@ data JS
-- |
-- A numeric literal
--
= JSNumericLiteral (Either Integer Double)
= JSNumericLiteral (Maybe SourceSpan) (Either Integer Double)

This comment has been minimized.

Copy link
@paf31

paf31 Jan 31, 2016

Contributor

Maybe it makes sense to add a type argument here?

This comment has been minimized.

Copy link
@nwolverson

nwolverson Jan 31, 2016

Author Contributor

You mean JS a = JSNumericLiteral a (Either Integer Double) ? I guess that was my original thought, the type does appear everywhere so maybe an alias type JS = JS1 (Maybe SourceSpan) ?

@paf31
Copy link
Contributor

paf31 commented Jan 31, 2016

Looks great so far! I'll take another look tomorrow. What sort of issues are you seeing with position info? Is it possible to drop this into Chrome and use it in the debugger?

@nwolverson
Copy link
Contributor Author

nwolverson commented Jan 31, 2016

If you can run directly against CommonJS modules you can use this as-is (I'm doing that for example with atom plugin, would at least be the same in an Electron app). But I set up a demo using webpack which will combine purescript source maps with its own: https://github.com/nwolverson/purescript-sourcemap-test
screen shot 2016-01-31 at 11 16 19

The issue I'm seeing with positions is in eg a + b + c, for e.g. b I see a span for the entire expression, not investigated further why.

@paf31
Copy link
Contributor

paf31 commented Jan 31, 2016

Looks great! Any idea why the CI build is breaking?

I think we should aim to ship this in 0.8.1. What do you think?

@nwolverson
Copy link
Contributor Author

nwolverson commented Jan 31, 2016

Needed to fix versions of sourcemaps used since the version on stackage didn't satisfy the constraint - guess we could just drop the constraint, if you build with an old version and use the flag it will take all day - but I don't know how official binaries are built.

And can't derive Typeable on older ghc versions - not sure where this constraint came from as I seemed to need to add a bunch extra when I rebased onto master. Presume it's something to do with the phantom type. If you know the original change maybe it's possible to remove all these - maybe it was removed on master and I re-added it when I rebased?

@garyb
Copy link
Member

garyb commented Jan 31, 2016

I removed the Data/Typeable deriving when we started using DataKinds to distinguish between the various meanings of ProperName (that's what the older GHCs struggle with, the interaction of DataKinds and DeriveTypeable).

@nwolverson
Copy link
Contributor Author

nwolverson commented Jan 31, 2016

Thank @garyb, I guess dropping syb meant those were not required.

@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from 3637517 to f1ccb5f Jan 31, 2016
@paf31
Copy link
Contributor

paf31 commented Feb 1, 2016

@nwolverson So would you say this is ready to merge? If so, we should release it with 0.8.1.

@nwolverson
Copy link
Contributor Author

nwolverson commented Feb 1, 2016

Yes - with the proviso that I'm sure the accuracy of the maps could be improved and no doubt other tweaks, at a later date

@nwolverson nwolverson changed the title [WIP] Add Source Map support. Add Source Map support. Feb 1, 2016
@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from b892c38 to 5fda853 Feb 15, 2016

class (Monoid gen) => Emit gen where
emit :: String -> gen
addMapping :: (Maybe SourceSpan) -> gen

This comment has been minimized.

Copy link
@paf31

paf31 Feb 21, 2016

Contributor

Does it make sense to simplify this to addMapping :: SourceSpan -> gen and to send Nothing to mempty in a helper function instead?

This comment has been minimized.

Copy link
@nwolverson

nwolverson Feb 21, 2016

Author Contributor

Yep that seems cleaner.

@paf31
Copy link
Contributor

paf31 commented Feb 21, 2016

@garyb I'm going to merge this soon for inclusion in 0.8.1, unless you have any other comments?

Thanks for all the work on this @nwolverson!

@texastoland
Copy link

texastoland commented Feb 21, 2016

Wow this is going to be the biggest dot release ever including deriving and operator sections :shipit:

@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from 307a887 to c175541 Feb 21, 2016
@nwolverson
Copy link
Contributor Author

nwolverson commented Feb 21, 2016

OK, made that 1 change from last comment and rebased.

@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from c175541 to c632cb4 Feb 22, 2016
@nwolverson
Copy link
Contributor Author

nwolverson commented Feb 22, 2016

Not sure why those builds timed out other than 1 which blew up on memory. How about now...

- Add sourcemaps license. Manually removed a few deletions
- Use Emit typeclass, move JS sourcespans into each ctor
- Remove sourcemaps from stackage build plans for travis, version is too old
- Keep source positions for literals
@nwolverson nwolverson force-pushed the nwolverson:sourcemap-pr branch from c632cb4 to 849459e Feb 22, 2016
@nwolverson
Copy link
Contributor Author

nwolverson commented Feb 23, 2016

As discussed on IRC this is hanging locally since recent rebase; it seems to be hanging on printing errors. I don't see the same on master but it's odd, I have no errors changes.

@paf31
Copy link
Contributor

paf31 commented Feb 24, 2016

My guess is that this is due to a missing pattern match in the Pretty modules.

Edit: @garyb Looks like your new data constructors are missing in prettyPrintBinder. It's odd though that they aren't there in prettyPrintValue, so maybe something is being desugared earlier there, or maybe it's not an issue.

@nwolverson
Copy link
Contributor Author

nwolverson commented Feb 24, 2016

So I added a position to atoms to improve source mapping, this seems to have caused infinite recursion between prettyPrintValueAtom and prettyPrintValue, I'm seeing Parens and BinaryNoParens ctors - the extra PositionedValue may have prevented some desugaring? Removing this for now.

@paf31
Copy link
Contributor

paf31 commented Feb 24, 2016

@nwolverson Having that extra constructor in there should be fine. I think you can just add the extra pass-through case to prettyPrintValue(Atom)

paf31 added a commit that referenced this pull request Feb 24, 2016
Add Source Map support.
@paf31 paf31 merged commit 5a87688 into purescript:master Feb 24, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@paf31
Copy link
Contributor

paf31 commented Feb 24, 2016

Thank you very much!

@codygman
Copy link

codygman commented Mar 4, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.