Skip to content

Conversation

@AviAvni
Copy link
Contributor

@AviAvni AviAvni commented Mar 27, 2018

@dsyme I did the same thing as for Entity and Val and the build failed and I can't find why
please help

member x.IsLinked = match box x.typar_attribs with null -> false | _ -> true
member x.IsLinked =
match x.typar_stamp with
| 0L -> false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a tiny bit dubious - 0 is a valid stamp, but perhaps it's never used

@dsyme
Copy link
Contributor

dsyme commented Mar 27, 2018

@AviAvni I edited your PR to try to see if stamp 0 was the problem

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@AviAvni It looks like the same Typar can have .Link called multiple times. This is somewhat unexpected, but I've adjusted the code to do what it used to do logically speaking in this case

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@AviAvni Ugh, still doesn't work

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

OK, copyTypar needed to clone the mutable data, see commits above. Ugly!

@smoothdeveloper
Copy link
Contributor

Can we consider a bit of formatting in copyTypar? It will make the ugly code a bit easier to read.

let copyTypar (tp: Typar) = 
    let optData =
        // Be careful to clone the mutable optional data too
        match tp.typar_opt_data with
        | None -> None
        | Some tg -> 
            Some
                ({ 
                    typar_il_name = tg.typar_il_name
                    typar_xmldoc = tg.typar_xmldoc
                    typar_constraints = tg.typar_constraints 
                })
                
    Typar.New 
        { tp with 
            typar_stamp = newStamp()
            typar_astype = Unchecked.defaultof<_>
            typar_opt_data = optData
        } 

(notepad edit)

@AviAvni
Copy link
Contributor Author

AviAvni commented Mar 28, 2018

@dsyme thank you very much I'll continue the work and post the statistics soon
@smoothdeveloper thanks I'll beautify copyTypar

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@smoothdeveloper thanks I'll beautify copyTypar

I think just use an explicit full copy (no copy-and-update { x with y ..})

@AviAvni
Copy link
Contributor Author

AviAvni commented Mar 28, 2018

@dsyme changed to full copy

@AviAvni
Copy link
Contributor Author

AviAvni commented Mar 28, 2018

@dsyme here are the statistics for fsharp.compiler.fsproj
the code for the measurment master...AviAvni:fcs-memory-3-measurment

~~~ Typar statistics: WithOptData 478976, WithoutOptData 254862
~~~ Typar statistics: typar_il_name: 0
~~~ Typar statistics: typar_xmldoc: 0
~~~ Typar statistics: typar_constraints: 66372
~~~ Typar statistics: typar_attribs: 1192

@AviAvni
Copy link
Contributor Author

AviAvni commented Mar 28, 2018

@dotnet-bot test this please

1 similar comment
@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@dotnet-bot test this please

@AviAvni
Copy link
Contributor Author

AviAvni commented Mar 28, 2018

@dsyme updated statistics

~~~ Typar statistics: WithOptData 66920, WithoutOptData 666474
~~~ Typar statistics: typar_il_name: 0
~~~ Typar statistics: typar_xmldoc: 0
~~~ Typar statistics: typar_constraints: 66456
~~~ Typar statistics: typar_attribs: 1190

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@AviAvni great!

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@dotnet-bot test this please

@AviAvni AviAvni closed this Mar 28, 2018
@AviAvni AviAvni reopened this Mar 28, 2018
@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@dotnet-bot test this please

@dsyme
Copy link
Contributor

dsyme commented Mar 28, 2018

@AviAvni Tests triggered now :)

@dsyme dsyme changed the title [WIP] Reduce memory footprint for Typar type [CompilerPerf] Reduce memory footprint for Typar type Mar 28, 2018
@auduchinok auduchinok mentioned this pull request Mar 28, 2018
10 tasks
@AviAvni
Copy link
Contributor Author

AviAvni commented Apr 5, 2018

@dsyme can this be merged or there is something to do?

@realvictorprm
Copy link
Contributor

@AviAvni i guess being at Fsharpx prevents us :p

@KevinRansom
Copy link
Contributor

@dotnet-bot test this please

@cartermp
Copy link
Contributor

Merging as per approvals.

@cartermp cartermp merged commit bc725e7 into dotnet:master Apr 13, 2018
@cartermp
Copy link
Contributor

Thanks for this awesome work (as always 😄), @AviAvni

auduchinok pushed a commit to auduchinok/fsharp that referenced this pull request Apr 18, 2018
* move typar_il_name and typar_xmldoc to OptionalData

* try move typar_constraints to OptionalData

* Update tast.fs

* update typar data twice correctly

* Update tast.fs

* fix

* move typar_attribs to OptionalData

* simplify code

* fix space

* fix SetConstraints and SetAttribs to free unused TyparOptionalData
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.

6 participants