Update for v0.7 support #13

ScottPJones · 2018-05-09T20:44:48Z

Please take a look - there are 5 broken tests in v0.7, it seems all because the v0.7 compiler is smarter, and in your test cases, the strings already just have a single copy (it seems it's better about making a single copy of a string, when there are more than one copy of the literal in a method)

codecov-io · 2018-05-09T21:49:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@335af70). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master    #13   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      1           
  Lines             ?     20           
  Branches          ?      0           
=======================================
  Hits              ?     20           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
src/InternedStrings.jl	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 335af70...481ba7d. Read the comment docs.

oxinabox · 2018-05-10T02:35:29Z

My thoughts on breaking string literal automatic interning is probabably to wrap them in a function that writes them to a tempfile and reads them back. Almost Impossible for that to optimise away. See also the use function in some other tests.

I see you've got rid of the core functionality function. Kinda makes sense. That was a legacy from when their was more type related stuff

ScottPJones · 2018-05-10T13:09:10Z

I also like to split things up, keep the top level module file pretty simple and include the rest, but when it's just a single file, and that short, it seemed like more hassle than it was worth.
I hope you didn't mind the reorg!

ScottPJones · 2018-05-10T16:59:48Z

If you think it's OK, please merge (I have a rule about not merging my own changes, even when I could, except when I'm the only person on a project 😄)

oxinabox · 2018-05-11T01:36:38Z

I didn't get time to properly review yesterday so I'll give it a second of today and then I merge

oxinabox

Some basic stuff, but more to come now I get what is happening in 0.7.
See following comment

oxinabox · 2018-05-11T05:19:38Z

.travis.yml

@@ -27,9 +27,9 @@ matrix:

 ## uncomment the following lines to override the default test script
 #script:


Oops something I missed when I originally renamed it, good catch

oxinabox · 2018-05-11T05:20:35Z

README.md

@@ -82,7 +86,7 @@ There is an issue though:
 How much are these tokens costing you in memory use?

 Originally you had say a 100MB (10⁸ bytes) text file (multiply this out as required).
-Which as a String took-up (10⁸ bytes + 1 pointer (4 bytes) + 1 length marker (4 bytes) + null terminating character (total 10⁸ + 9 bytes).
+Which as a String took-up (10⁸ bytes + 1 pointer (4 or 8 bytes) + 1 length marker (4 or 8 bytes) + null terminating character (total 10⁸ + 9 (or 17) bytes).


I think you mean (or 16)

That was a first pass and fixing the calculations.
The actual memory usage is a lot more complex.
The String type (in v0.6 and later) takes up (on 64-bit platforms) div(sizeof(str) + 1 + sizeof(Int) + 15, 16) * 16 (at a minimum, I'm not sure whether it can allocate 48 bytes, for example, I'll have to check, but it's always a multiple of 16 [I think that's a multiple of 8 on 32-bit systems])

oxinabox · 2018-05-11T05:21:37Z

.travis.yml

@@ -27,9 +27,9 @@ matrix:

 ## uncomment the following lines to override the default test script
 #script:
-#  - julia -e 'Pkg.clone(pwd()); Pkg.build("StringInterning"); Pkg.test("StringInterning"; coverage=true)'
+#  - julia -e 'Pkg.clone(pwd()); Pkg.build("InternedStrings"); Pkg.test("InternedStrings"; coverage=true)'
 after_success:
  # push coverage results to Coveralls


Can we use just CodeCov and not Coveralls?
I don't see the point in having both, and I'm a bit more used to CodeCov

That's fine, I've just copied that for that last 3 years from other packages!
I'll remove Coveralls.

oxinabox · 2018-05-11T05:23:39Z

src/InternedStrings.jl

+
+
+macro i_str(s)
+    true_string_expr = esc(Meta.parse(string('"', unescape_string(s), '"')))


A comment is likely wanted here explaining that is is making interpolation happen
Probably something I missed

Sure, will add.

oxinabox · 2018-05-11T05:24:07Z

src/InternedStrings.jl


 export @i_str, intern

-include("corefunctionality.jl")
-
-
 Base.@deprecate_binding(InternedString, String, true)
 #InternedString(s)=intern(String(s))


This commented out-line should be deleted, (that is my bad from before)

oxinabox · 2018-05-11T06:19:05Z

Also you've broken the ability to do julia test/corefunctionality.jl,
because the test files are nolonger valid in isolation

I kinda see why that is needful, since there is shared code now in runtests.jl
It will probably do.
It isn't like these tests take ages so you want to run them in isolation.
The alternative would be to define a test/helpers.jl and include that in each file.

Not the main point though that I wanted to add, see Next comment

oxinabox · 2018-05-11T06:25:18Z

So I believe most of what is going wrong in 0.7'st tests is
the result of JuliaLang/julia#22193

because it means for equal strings they will be === and have same object_id
So using object_id to compare them won't get one anywhere.

What wants to be done is to use pointer to compare them

julia> k = "AB"
"AB"

julia> m = string(Char(65))*"B"
"AB"

julia> pointer(k)
Ptr{UInt8} @0x00007fc26cb40df8

julia> pointer(m)
Ptr{UInt8} @0x00007fc26cb43658

julia> objectid(k)
0x4bc65251d1065455

julia> objectid(m)
0x4bc65251d1065455

julia> k===m
true

so rather than object_id_eq, lets have addr_eq(a, b) = pointer(a)==pointer(b).

And that will probably do right in 0.7.

If problems persist with literals being actually automatically the same reference,
replace the literals with v*"" which will break the automatic reference detection.


julia> bar() = pointer("a") == pointer("a")
bar (generic function with 1 method)

julia> bar()
true

julia> foo() = pointer("a"*"") == pointer("a")
foo (generic function with 1 method)

julia> foo()
false

quinnj

The move from corefunctionality.jl to InternedStrings.jl is annoying because it's hard to review what actually changed in the core methods, but this LGTM.

How serious are the newly introduced @test_brokens?

oxinabox · 2018-05-11T13:15:29Z

Ideally it would have been done in a separate PR but no big deal.
AFAICT there are no changes beyond the compat stuff at the top of the file..

The @test_brokens are problems with the tests, not with the code.
See #13 (comment)
They are all fixable and will be removed before this is merged (and I suspect the commit just made did so)

oxinabox · 2018-05-11T13:20:05Z

test/corefunctionality.jl


-        @test objectid(intern("Gold")) == target_id
+        @test pointer(intern(SubString("Gold", 1))) == target_addr


That is a different test.
A useful test, but a different one, because interning SubStrings works differently
However it doesn't exactly matter because the test was redundant anyway.
This whole test suite is basically checking the example from the readme works still.

And it is now out of sync with that, but I'll replace it anyway

Ah, I didn't realize that interning SubStrings was handled differently, I was just trying to break the connection between the string literals.

ScottPJones · 2018-05-11T13:32:01Z

I am surprised that str*"" isn't optimized away at some point.

oxinabox · 2018-05-11T13:36:34Z

One day it will be, but not today.
I guess the uniqueifying happens before constant propagation

Update for v0.7 support

b059333

ScottPJones requested a review from oxinabox May 9, 2018 20:45

oxinabox reviewed May 11, 2018

View reviewed changes

oxinabox mentioned this pull request May 11, 2018

0.7 failure #14

Closed

quinnj approved these changes May 11, 2018

View reviewed changes

Updates to address Lyndon's comments

8c796cc

oxinabox reviewed May 11, 2018

View reviewed changes

Make example use pointers

88e1434

CodeCov only, no coveralls [ci skip]

481ba7d

oxinabox merged commit 34e9c24 into master May 11, 2018

oxinabox mentioned this pull request May 11, 2018

Fix most deprecations on Julia 0.7 #15

Merged

ScottPJones deleted the spj/v07update branch May 11, 2018 23:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for v0.7 support #13

Update for v0.7 support #13

ScottPJones commented May 9, 2018 •

edited

codecov-io commented May 9, 2018 •

edited

oxinabox commented May 10, 2018

ScottPJones commented May 10, 2018

ScottPJones commented May 10, 2018

oxinabox commented May 11, 2018 •

edited

oxinabox left a comment

oxinabox May 11, 2018

oxinabox May 11, 2018

ScottPJones May 11, 2018

oxinabox May 11, 2018

ScottPJones May 11, 2018

oxinabox May 11, 2018 •

edited

ScottPJones May 11, 2018

oxinabox May 11, 2018

oxinabox commented May 11, 2018 •

edited

oxinabox commented May 11, 2018

quinnj left a comment

oxinabox commented May 11, 2018

oxinabox May 11, 2018

ScottPJones May 11, 2018

ScottPJones commented May 11, 2018

oxinabox commented May 11, 2018

		@@ -27,9 +27,9 @@ matrix:

		## uncomment the following lines to override the default test script
		#script:



		macro i_str(s)
		true_string_expr = esc(Meta.parse(string('"', unescape_string(s), '"')))


		@test objectid(intern("Gold")) == target_id
		@test pointer(intern(SubString("Gold", 1))) == target_addr

Update for v0.7 support #13

Update for v0.7 support #13

Conversation

ScottPJones commented May 9, 2018 • edited

codecov-io commented May 9, 2018 • edited

Codecov Report

oxinabox commented May 10, 2018

ScottPJones commented May 10, 2018

ScottPJones commented May 10, 2018

oxinabox commented May 11, 2018 • edited

oxinabox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox May 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented May 11, 2018 • edited

oxinabox commented May 11, 2018

quinnj left a comment

Choose a reason for hiding this comment

oxinabox commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottPJones commented May 11, 2018

oxinabox commented May 11, 2018

ScottPJones commented May 9, 2018 •

edited

codecov-io commented May 9, 2018 •

edited

oxinabox commented May 11, 2018 •

edited

oxinabox May 11, 2018 •

edited

oxinabox commented May 11, 2018 •

edited